Skip to content

Delay astroid_bootstrapping() until instantiating AstroidBuilder #2210

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
Jun 12, 2023

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Reduce time to import astroid by delaying astroid_bootstrapping() until the first instantiation of AstroidBuilder.

Closes #2161

Benchmark

python3 -X importtime -c 'import astroid'

main@ 4493399

import time:     34604 |     315816 | astroid

pr

import time:     14397 |      75213 | astroid

@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a5 milestone Jun 11, 2023
@jacobtylerwalls jacobtylerwalls added the pylint-tested PRs that don't cause major regressions with pylint label Jun 11, 2023
@codecov
Copy link

codecov bot commented Jun 11, 2023

Codecov Report

Merging #2210 (8f78173) into main (1fbbf25) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2210   +/-   ##
=======================================
  Coverage   92.68%   92.69%           
=======================================
  Files          94       94           
  Lines       10828    10835    +7     
=======================================
+ Hits        10036    10043    +7     
  Misses        792      792           
Flag Coverage Ξ”
linux 92.45% <100.00%> (+<0.01%) ⬆️
pypy 87.65% <100.00%> (+<0.01%) ⬆️
windows 92.28% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Ξ”
astroid/brain/brain_builtin_inference.py 91.64% <100.00%> (+0.01%) ⬆️
astroid/brain/brain_nose.py 95.00% <100.00%> (+0.12%) ⬆️
astroid/builder.py 94.19% <100.00%> (+0.05%) ⬆️
astroid/raw_building.py 94.96% <100.00%> (+0.04%) ⬆️

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Wow, 4 Time faster ? Or at least 2 times ? Not sure if I read the data corectly. It seems you did not have to add lazy anything inside the astroid manager either ? I'm going to check if that's yet another way to make it faster in the future once I'm not on mobile. Exciting change either ways ! The impact will be enormous when pylint is launched in parallele on small files by an external tool like pre-commit !

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Doesn't it make more sense for the bootstrapping to occur when AstroidManager is created? Its name (to me) implies that I would be responsible for this instead of the Builder.

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Jun 12, 2023

Doesn't it make more sense for the bootstrapping to occur when AstroidManager is created? Its name (to me) implies that I would be responsible for this instead of the Builder.

This would create an infinite loop, since calling _astroid_bootstrapping() from AstroidManager.__init__() would create a Builder, which creates a Manager:

def _astroid_bootstrapping() -> None:
"""astroid bootstrapping the builtins module"""
# this boot strapping is necessary since we need the Const nodes to
# inspect_build builtins, and then we can proxy Const
builder = InspectBuilder()
astroid_builtin = builder.inspect_build(builtins)

class InspectBuilder:
"""class for building nodes from living object
this is actually a really minimal representation, including only Module,
FunctionDef and ClassDef nodes and some others as guessed.
"""
def __init__(self, manager_instance: AstroidManager | None = None) -> None:
self._manager = manager_instance or AstroidManager()

Also, I like the idea of keeping this guard closer to what it actually needs to guard--the building of the stdlib ast by the Builder.

@DanielNoord
Copy link
Collaborator

Doesn't it make more sense for the bootstrapping to occur when AstroidManager is created? Its name (to me) implies that I would be responsible for this instead of the Builder.

This would create an infinite loop, since calling _astroid_bootstrapping() from AstroidManager.__init__() would create a Builder, which creates a Manager:

def _astroid_bootstrapping() -> None:
"""astroid bootstrapping the builtins module"""
# this boot strapping is necessary since we need the Const nodes to
# inspect_build builtins, and then we can proxy Const
builder = InspectBuilder()
astroid_builtin = builder.inspect_build(builtins)

class InspectBuilder:
"""class for building nodes from living object
this is actually a really minimal representation, including only Module,
FunctionDef and ClassDef nodes and some others as guessed.
"""
def __init__(self, manager_instance: AstroidManager | None = None) -> None:
self._manager = manager_instance or AstroidManager()

Also, I like the idea of keeping this guard closer to what it actually needs to guard--the building of the stdlib ast by the Builder.

Sounds fine! Thanks for explaining.

@jacobtylerwalls jacobtylerwalls merged commit 33d0e84 into pylint-dev:main Jun 12, 2023
@jacobtylerwalls jacobtylerwalls deleted the speed-import branch June 12, 2023 21:31
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Jun 13, 2023

Just tested the changes and it look like it's almost 3 time faster, this is great work !

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.

Make import astroid faster (again)
3 participants