Skip to content

bpo-43392: Optimize repeated calls to __import__()#24735

Merged
miss-islington merged 1 commit into
python:mainfrom
Kronuz:master
Aug 12, 2021
Merged

bpo-43392: Optimize repeated calls to __import__()#24735
miss-islington merged 1 commit into
python:mainfrom
Kronuz:master

Conversation

@Kronuz

@Kronuz Kronuz commented Mar 3, 2021

Copy link
Copy Markdown
Contributor

Implements a two steps check in importlib._bootstrap._find_and_load() to avoid locking when the module has been already imported and it's ready.


Using importlib.__import__(), after this, does show a big difference:

Before:

$ ./python -c 'import timeit; print(timeit.timeit("__import__(\"timeit\")", setup="from importlib import __import__"))'
15.92248619502061

After:

$ ./python -c 'import timeit; print(timeit.timeit("__import__(\"timeit\")", setup="from importlib import __import__"))'
1.206068897008663

https://bugs.python.org/issue43392

Automerge-Triggered-By: GH:pitrou

@github-actions

github-actions Bot commented Apr 4, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 4, 2021
@DinoV DinoV requested a review from brettcannon April 6, 2021 23:30

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

The optimization looks reasonable in the principle, as there's already a similar one on the C side.

Comment thread Lib/importlib/_bootstrap.py Outdated
Comment thread Lib/importlib/_bootstrap.py Outdated
@bedevere-bot

Copy link
Copy Markdown

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.

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Apr 17, 2021
@brettcannon

Copy link
Copy Markdown
Member

@Kronuz if this is ready for review again then please see the instructions at #24735 (comment) on how to request a re-review.

@Kronuz

Kronuz commented Jun 11, 2021

Copy link
Copy Markdown
Contributor Author

Oh! @brettcannon, yes I have made the requested changes; please review again. I just missed that part of the instructions.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@Kronuz

Kronuz commented Jun 25, 2021

Copy link
Copy Markdown
Contributor Author

Didn't close it... reopening 🤪

@Kronuz

Kronuz commented Jul 7, 2021

Copy link
Copy Markdown
Contributor Author

@pitrou, I've addressed the suggestions and made the changes, however I'm not sure if I'm doing something wrong or why this doesn't show as all green and instead has 2 workflows awaiting approval and the "Changes requested" status. Please let me know if I'm missing something.

@pitrou

pitrou commented Jul 7, 2021

Copy link
Copy Markdown
Member

@Kronuz You just had to post the phrase "I have made the requested changes; please review again" as mentioned by the bot above :)

As for the CI steps, they must be approved by a core developer since this is your first PR.

@pitrou

pitrou commented Jul 7, 2021

Copy link
Copy Markdown
Member

@pablogsal @iritkatriel Do you want to take a look at this?

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

Minor comment tweaks, but the PR doesn't allow for modification by me so I can't fix them.

Otherwise importlib.h is out-of-date. Let's give people a week to add any review they may want, otherwise I will merge this once importlib.h is no longer a merge conflict.

Comment thread Misc/NEWS.d/next/Library/2021-03-03-13-32-37.bpo-43392.QQumou.rst Outdated
Comment thread Lib/importlib/_bootstrap.py Outdated
@bedevere-bot

Copy link
Copy Markdown

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.

Implements a two steps check in `importlib._bootstrap._find_and_load()` to avoid locking when the module has been already imported and it's ready.
@Kronuz

Kronuz commented Aug 12, 2021

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

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

This looks good to me.

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.

6 participants