Skip to content
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

Speed up import time by deferring inspect #499

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Speed up import time by deferring inspect #499

merged 4 commits into from
Aug 20, 2024

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Aug 5, 2024

Deferring import of inspect cuts the import time by ~10% (4ms on my machine).

CPython issue: python/cpython#118761

I haven't been able to run the tests locally, seeing these errors:

RROR tests/test_api.py - AttributeError: module 'tests.fixtures' has no attribute 'EggInfoPkg'
ERROR tests/test_integration.py - AttributeError: module 'tests.fixtures' has no attribute 'DistInfoPkg'
ERROR tests/test_main.py
ERROR tests/test_zip.py - AttributeError: module 'tests.fixtures' has no attribute 'ZipFixtures'

Benchmarks

These have been run with latest CPython main branch (as of Aug 6th 2024), these gains are likely representative for Python 3.13, but not 3.12.

this PR

hyperfine -w 5 'python -c "import importlib_metadata"'
Benchmark 1: python -c "import importlib_metadata"
  Time (mean ± σ):      39.6 ms ±   4.6 ms    [User: 29.8 ms, System: 9.4 ms]
  Range (min … max):    29.7 ms …  49.8 ms    68 runs

main

hyperfine -w 5 'python -c "import importlib_metadata"'
Benchmark 1: python -c "import importlib_metadata"
  Time (mean ± σ):      43.4 ms ±   3.6 ms    [User: 33.1 ms, System: 9.8 ms]
  Range (min … max):    34.9 ms …  50.1 ms    68 runs

@@ -1071,6 +1070,9 @@ def _topmost(name: PackagePath) -> Optional[str]:
return top if rest else None


inspect = None
Copy link
Contributor Author

@danielhollas danielhollas Aug 5, 2024

Choose a reason for hiding this comment

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

This is a trick that is used in stdlib. I am not sure it is needed here, but since get_toplevel_name is called in a loop from _top_level_inferred perhaps it is warranted to avoid the overhead of calling import inspect repeatedly.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of global variables or the additional complexity. This change introduces enough disruption to the essential logical flow that I'm -1. Do we know how much overhead there is in repeated import inspect? My understanding (which may be incorrect) is that import inspect is essentially a dict lookup if it's already been imported. I'm guessing the overhead is acceptable. Can we try a simple deferral for now?

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Thanks for the contrib!

@@ -1071,6 +1070,9 @@ def _topmost(name: PackagePath) -> Optional[str]:
return top if rest else None


inspect = None
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of global variables or the additional complexity. This change introduces enough disruption to the essential logical flow that I'm -1. Do we know how much overhead there is in repeated import inspect? My understanding (which may be incorrect) is that import inspect is essentially a dict lookup if it's already been imported. I'm guessing the overhead is acceptable. Can we try a simple deferral for now?


global inspect
if inspect is None:
import inspect
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't have a test protecting this behavior, I'd like to see a comment pointing to the issue, so a future someone doesn't refactor this optimization away.

@jaraco jaraco closed this Aug 19, 2024
@jaraco jaraco reopened this Aug 19, 2024
@jaraco jaraco merged commit 2c43cfe into python:main Aug 20, 2024
12 checks passed
@jaraco
Copy link
Member

jaraco commented Aug 20, 2024

I've minimized the change to address the specific issue. I'll deal with the typeshed ignore workaround separately.

@jaraco
Copy link
Member

jaraco commented Aug 20, 2024

I should have added a news fragment before merging. I added it later in 71b4678.

@jaraco
Copy link
Member

jaraco commented Aug 20, 2024

This change is released in v8.4.0.

@danielhollas danielhollas deleted the defer-inspect branch August 25, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants