gh-103200: Fix performance issues with zipimport.invalidate_caches()

[desmondcheongzx]

This PR fixes the over-eagerness of the original zipimport.invalidate_caches() implementation.

Currently in zipimport.invalidate_caches(), the cache of zip files is repopulated at the point of invalidation. This causes cache invalidation to be slow, and violates the semantics of cache invalidation which should simply clear the cache. Cache repopulation should occur on the next access of files.

There are three relevant events to consider:

1. The cache is accessed while valid
2. invalidate_caches() is called
3. The cache is accessed after being invalidated

Events (1) and (2) should be fast, while event (3) can be slow since we're repopulating a cache. In the original implementation, (1) and (3) are fast, but (2) is slow.

This PR shifts the cost of reading the directory out of cache invalidation and back to cache access, while avoiding any behaviour change introduced in Python 3.10+ and keeping the common path of reading the cache performant.

Ideally, this fix should be backported to Python 3.10+.

• Issue: zipimport.invalidate_caches() implementation causes performance regression for importlib.invalidate_caches() #103200

[ghost]

All commit authors signed the Contributor License Agreement.

[brettcannon]

I'm concerned there's a disconnect between the cache being marked as dirty in an instance of zipimporter and _zip_directory_cache.

If you were to create two instances of zipimporter, call invalidate_caches() on one instance but never use it to cause _get_files() to be called, and then continue to using the second instance, the supposedly poisoned cache will continue to be used by the second instance.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[desmondcheongzx]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

[brettcannon]

LGTM! I tweaked some comments to be have proper punctuation and updated the branch. I have turned on auto-merge so this should go in once CI passes!

[desmondcheongzx]

Thanks @brettcannon, I appreciate it!

[AdamWill]

I think this may have caused a failure in the dask/distributed test suite: dask/distributed#8708 . I'm not sure if that means it's a bug in cpython, or just the distributed test suite doing something weird. The test suite does this:

        for value in [123, 456]:
            with tmp_text(
                "myfile.py", f"def f():\n    return {value}"
            ) as fn_my_file:
                with zipfile.ZipFile("myfile.zip", "w") as z:
                    z.write(fn_my_file, arcname=os.path.basename(fn_my_file))
                await c.upload_file("myfile.zip")

                x = c.submit(g, pure=False)
                result = await x
                assert result == value

the c.upload_file call triggers this bit of distributed.utils.import_file():

if ext in (".egg", ".zip", ".pyz"):
    if path not in sys.path:
        sys.path.insert(0, path)
    names = (mod_info.name for mod_info in pkgutil.iter_modules([path]))
    names_to_import.extend(names)

and that pkgutil.iter_modules([path]) blows up in iter_zipimport_modules, which accesses zipimport._zip_directory_cache directly:

        def iter_zipimport_modules(importer, prefix=''):
>           dirlist = sorted(zipimport._zip_directory_cache[importer.archive])
E           KeyError: '/tmp/dask-scratch-space/scheduler-belyj1dp/myfile.zip'

Note the somewhat odd thing the test does is to upload two different files with the same name in succession. The failure only happens in that case. If we make the test upload only once, or use a different filename each time, this doesn't happen.

[AdamWill]

If I change the dirlist = line in pkgutil.py to be this, the test passes:

dirlist = sorted(zipimporter(importer.archive)._get_files())

[AdamWill]

oh, forgot to mention, this test is all async. I tried building a minimal test reproducer that's synchronous, but it doesn't reproduce. I'll try doing an async reproducer tomorrow...

[desmondcheongzx]

@AdamWill while using dirlist = sorted(zipimporter(importer.archive)._get_files()) works because the _get_files() method handles the keyError, I'm not sure how I feel about a private API being used. We should either make it public, or fix pkgutil. I'm leaning towards the latter because public APIs should not break user code.

Could you file an issue against this and tag me?