Skip to content

Prevent cache misses in do_import_module() #2206

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

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Jun 7, 2023

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

If the modname argument is None, which it is a lot, since it's the default, it's okay to use the cache; there's no point in recreating the requested module every time.

Closes #2041

See discussion on #2041 and duplicate issue #2124 for how this was sourced to #1747.

Not suggesting a backport or changelog; there's an argument that the astroid cache is public, but not a slam-dunk one, hence, this is really just a performance optimization I'd suggest just describing with all the 3.0 performance changes (which I suggest doing when the release is finalized).

If modname is None (default), it's okay to use the cache;
there's no point in recreating the module ad nauseum.

Closes pylint-dev#2041

Co-authored-by: Leandro T. C. Melo <[email protected]>
@jacobtylerwalls jacobtylerwalls added topic-performance pylint-tested PRs that don't cause major regressions with pylint labels Jun 7, 2023
@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a5 milestone Jun 7, 2023
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #2206 (2add5ab) into main (bd78ab0) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2206      +/-   ##
==========================================
- Coverage   92.68%   92.68%   -0.01%     
==========================================
  Files          94       94              
  Lines       10818    10820       +2     
==========================================
+ Hits        10027    10028       +1     
- Misses        791      792       +1     
Flag Coverage Ξ”
linux 92.43% <100.00%> (-0.01%) ⬇️
pypy 87.64% <100.00%> (-0.01%) ⬇️
windows 92.27% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Ξ”
astroid/nodes/_base_nodes.py 98.09% <100.00%> (ΓΈ)

... and 2 files with indirect coverage changes

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.

Do we want a test for this? This feels like the kind of stuff we don't fully understand in 3/4 years time?

@jacobtylerwalls jacobtylerwalls merged commit 4493399 into pylint-dev:main Jun 7, 2023
@jacobtylerwalls jacobtylerwalls deleted the use-cache-no-mod-name branch June 7, 2023 21:47
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.

regression introduced in 2.13.0 (from 2.12.14) concerning use_cache in astroid's cache
2 participants