Skip to content

Speed up import time by deferring inspect#499

Merged
jaraco merged 4 commits into
python:mainfrom
danielhollas:defer-inspect
Aug 20, 2024
Merged

Speed up import time by deferring inspect#499
jaraco merged 4 commits into
python:mainfrom
danielhollas:defer-inspect

Conversation

@danielhollas

@danielhollas danielhollas commented Aug 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

danielhollas
danielhollas commented Aug 5, 2024
View reviewed changes
Comment thread importlib_metadata/__init__.py Outdated
return top if rest else None


inspect = None

@danielhollas danielhollas Aug 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

@jaraco jaraco Aug 19, 2024

Copy link
Copy Markdown
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?

@danielhollas danielhollas marked this pull request as ready for review August 6, 2024 02:26
@danielhollas danielhollas mentioned this pull request Aug 6, 2024
Closed
jaraco
jaraco reviewed Aug 19, 2024
View reviewed changes

@jaraco jaraco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contrib!

Comment thread importlib_metadata/__init__.py Outdated
return top if rest else None


inspect = None

@jaraco jaraco Aug 19, 2024

Copy link
Copy Markdown
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?

Comment thread importlib_metadata/__init__.py Outdated

global inspect
if inspect is None:
import inspect

@jaraco jaraco Aug 19, 2024

Copy link
Copy Markdown
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
@jaraco

jaraco commented Aug 20, 2024

Copy link
Copy Markdown
Member

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

@jaraco

jaraco commented Aug 20, 2024

Copy link
Copy Markdown
Member

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

@jaraco

jaraco commented Aug 20, 2024

Copy link
Copy Markdown
Member

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

Reviewers

@jaraco jaraco jaraco left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@danielhollas @jaraco