FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-132952: Import _io instead of io at startup by JelleZijlstra · Pull Request #132957 · python/cpython · GitHub

Repository navigation

gh-132952: Import _io instead of io at startup - #132957

Merged
JelleZijlstra merged 6 commits into
python:mainfrom
JelleZijlstra:sunder-io
Apr 28, 2025
Merged

JelleZijlstra merged 6 commits into
python:mainfrom
JelleZijlstra:sunder-io

Conversation

JelleZijlstra commented Apr 25, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member Author

Like #132956 this only helps -S startup because io gets imported anyway by site.py and possibly other places. Maybe we can make those other places also use _io but not convinced it's worth it.

mdboom commented Apr 25, 2025

Copy link
Copy Markdown
Contributor

I'm going to run this branch on our benchmarking hardware (as well as #132956), just so we have some numbers.

mdboom commented Apr 26, 2025

Copy link
Copy Markdown
Contributor

Benchmarking of this PR: https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20250425-3.14.0a7%2B-dac50cc/bm-20250425-linux-x86_64-JelleZijlstra-sunder_io-3.14.0a7%2B-dac50cc-vs-base.svg

This is actually more effective than (19%) than #132956 (12%). I think it makes sense to merge this, and maybe the other just for good measure (though it should have no additional effect).

JelleZijlstra marked this pull request as ready for review April 26, 2025 16:06

Copy link
Copy Markdown
Member Author

This is now ready.

One tricky aspect is that we need to check any side effects that come from importing io. Most of the file is defining ABCs and registering things to those ABCs; it's fine if we omit that because the ABCs don't exist if the module isn't imported. However, it also sets the __module__ attribute on the UnsupportedOperation exception. I changed the code to set that attribute in _io instead because otherwise users who never import io would see this exception with the wrong module name.

@@ -0,0 +1,2 @@
Speed up startup with the ``-S`` argument by about 19% by importing the

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

it is worth mentioning "vs what" when stating an improvement %.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's 19% vs. before this commit, but it's only 1.5% faster than 3.13.0 (which is probably what most users care about, since they don't run main). I would argue 1.5% is in the noise, so we probably shouldn't have a what's new entry at all -- this is just counteracting a regression in 3.14 development.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's only a NEWS entry, not text in the What's New, so I'll remove the 19% claim but leave the NEWS entry since this could affect behavior some users care about.

mdboom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, but I think we should remove the whatsnew.

bedevere-app Bot commented Apr 28, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

bedevere-app Bot commented Apr 28, 2025

Copy link
Copy Markdown

Thanks for making the requested changes!

@mdboom, @gpshead: please review the changes made to this pull request.

bedevere-app Bot requested review from gpshead and mdboom April 28, 2025 13:33
JelleZijlstra merged commit 58567cc into python:main Apr 28, 2025
JelleZijlstra deleted the sunder-io branch April 28, 2025 15:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

3 participants


Back | FazBrowse Home | New Git URL