Skip to content

Optimize _compute_mro() #2187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 20, 2023
Merged

Conversation

jacobtylerwalls
Copy link
Member

Type
βœ“ πŸ”¨ Refactoring

Description

  • Optimize clean_duplicates_mro()
  • Short-circuit in _compute_mro() for "builtins.object"

Performance improvement is more noticeable in projects making heavy use of inheritance. For the project music21 from the pylint primer:

Benchmark

from pylint.lint import Run
Run(['music21'], exit=False)  # using project's own rcfile

main

452811674 function calls (383785563 primitive calls) in 161.566 seconds
ncalls  tottime  percall  cumtime  percall filename:lineno(function)
...
427785/97707    0.934    0.000   10.187    0.000 scoped_nodes.py:2808(_compute_mro)

PR

450362344 function calls (381157337 primitive calls) in 157.018 seconds
ncalls  tottime  percall  cumtime  percall filename:lineno(function)
...
427785/97707    0.692    0.000    8.160    0.000 scoped_nodes.py:2808(_compute_mro)

That's ~2s saved out of ~160s, or 1.25%.

@jacobtylerwalls jacobtylerwalls added topic-performance pylint-tested PRs that don't cause major regressions with pylint labels May 20, 2023
@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a4 milestone May 20, 2023
@jacobtylerwalls jacobtylerwalls changed the title Optimize clean_duplicates_mro() Optimize _compute_mro() May 20, 2023
@codecov
Copy link

codecov bot commented May 20, 2023

Codecov Report

Merging #2187 (81a4d9b) into main (feafcb8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2187      +/-   ##
==========================================
+ Coverage   92.61%   92.63%   +0.02%     
==========================================
  Files          94       94              
  Lines       10814    10818       +4     
==========================================
+ Hits        10015    10021       +6     
+ Misses        799      797       -2     
Flag Coverage Ξ”
linux 92.39% <100.00%> (+0.02%) ⬆️
pypy 87.59% <100.00%> (+0.02%) ⬆️
windows 92.22% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
astroid/nodes/scoped_nodes/scoped_nodes.py 91.96% <100.00%> (+0.02%) ⬆️

... and 1 file with indirect coverage changes

@@ -133,24 +133,24 @@ def clean_typing_generic_mro(sequences: list[list[ClassDef]]) -> None:
bases_mro.pop(position_in_inferred_bases)


def clean_duplicates_mro(sequences, cls, context):
def clean_duplicates_mro(
sequences: Iterable[Iterable[ClassDef]],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yield from baseobj.bases

This makes this SuccesfulInferenceResult although I wonder if ClassDef.bases could be narrowed down. I'd be okay with making this ClassDef for now and fixing later if necessary, but note that this will add one more mypy warning after _inferred_bases gets its signature typed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. However, Pyright also says it could be ClassDef due to this:

if not baseobj.hide:
yield baseobj

Let's wait until we're ready to type this all together.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah of course, the bases line just broadens the type (perhaps/probably incorrectly)

@jacobtylerwalls jacobtylerwalls merged commit f21b8c2 into pylint-dev:main May 20, 2023
@jacobtylerwalls jacobtylerwalls deleted the optimize-mro branch May 20, 2023 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pylint-tested PRs that don't cause major regressions with pylint topic-performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants