-
-
Notifications
You must be signed in to change notification settings - Fork 32k
bpo-14678: Update zipimport to support importlib.invalidate_caches() #24159
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
miss-islington
merged 11 commits into
python:master
from
desmondcheongzx:bpo-14678-add-invalidate-caches-to-zipimporter
Mar 8, 2021
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
41c3657
Add a get_files() method to zipimporter
desmondcheongzx 4e99b50
Add an invalidate_caches() method to zipimporter
desmondcheongzx 1bbd671
Add a news entry
desmondcheongzx e51cb08
Add an additional test for zipimporter.invalidate_caches()
desmondcheongzx ccb6c70
Rename get_files() to _get_files()
desmondcheongzx ffca660
Consolidate updates of _files into invalidate_caches()
desmondcheongzx 5ce2ada
Remove stray newline
desmondcheongzx d7302fb
Add author to news entry
desmondcheongzx b33b9dc
Merge remote-tracking branch 'upstream/master' into bpo-14678-add-inv…
desmondcheongzx bf52347
Move tests for invalidate_caches into a new method
desmondcheongzx 64d9637
Update bytecode for importlib._zipimport.h
desmondcheongzx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 3 additions & 0 deletions
3
Misc/NEWS.d/next/Library/2021-01-07-21-25-49.bpo-14678.1zniCH.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Add an invalidate_caches() method to the zipimport.zipimporter class to | ||
support importlib.invalidate_caches(). | ||
desmondcheongzx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Patch by Desmond Cheong. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @desmondcheongzx. I do appreciate this fix.
Just dropping a comment in case the impact of this was missed during the review.
This might impact the performance notably. For example, I only import two zipped packages
py4j
andpyspark
(and of course I have many packages installed from pip, and import them within my code) but it slows down the speed ofimportlib.invalidate_caches
up to 3500% because now it has to go through the zipped package.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was missed unfortunately.
To get everyone back up to speed, in the original discussion there were two decisions made:
_get_files()
method should be as fast as possible in the common case where zip files are not changed. This is why we avoided a stat call on every access of the importer.invalidate_caches()
is called. This second decision might not be the right one, because clearing a cache does not mean that we want to immediately repopulate it.There are three relevant events:
_get_files()
is called with a valid cacheinvalidate_caches
is called_get_files()
is called with an invalid cacheIMO, what we really want is for events (1) and (2) to be fast, while event (3) can be slow since we're repopulating a cache. (Note that in the original PR we made (1) and (3) fast, but (2) slow). So I propose the following:
cache_is_valid
that is set to false wheninvalidate_caches()
is called._get_files()
, ifcache_is_valid
is true, use the cache. Ifvalid_cache
is false, call_read_directory()
.This approach avoids any behaviour change and keeps the common path performant, while also shifting the cost of reading the directory out of cache invalidation.
If we want to go further and consider the fact that we rarely expect zip archives to change, then we can also consider adding a flag to
importlib.invalidate_caches
so that users can choose whether zip caches are invalidated.Any thoughts, @brettcannon?