GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Speed up import time by deferring inspect by danielhollas · Pull Request #499 · python/importlib_metadata

Conversation

@danielhollas

@danielhollas danielhollas commented Aug 5, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@danielhollas
Defer import inspect
8e66fbc
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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jaraco jaraco Aug 19, 2024

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@danielhollas
danielhollas marked this pull request as ready for review August 6, 2024 02:26
@danielhollas danielhollas mentioned this pull request Aug 6, 2024
Improve import time of various stdlib modules python/cpython#118761
Closed
jaraco
jaraco reviewed Aug 19, 2024
View reviewed changes

@jaraco jaraco left a comment

Copy link
Copy Markdown
Member

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 for the contrib!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jaraco jaraco closed this Aug 19, 2024
@jaraco jaraco reopened this Aug 19, 2024
danielhollas and others added 3 commits August 20, 2024 12:23
@danielhollas
Don't use global var
debb516 
- wallrusify
- add a note about deffered import
@jaraco
Restore single-expression logic.
e99c105
@jaraco
Use third-person imperative voice and link to issue in comment.
a7aaf72
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jaraco

jaraco commented Aug 20, 2024

Copy link
Copy Markdown
Member

This change is released in v8.4.0.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@danielhollas @jaraco
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.