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

When extracting a wheel, do not recheck parent directories #12782

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

morotti
Copy link
Contributor

@morotti morotti commented Jun 21, 2024

Hello,

Can i offer you a patch to make pip install ~5% faster?
Counting 5% faster in average. Closer to 10% faster on slow disks or network volumes, closer to 1% faster on fast SSD.

The fix is pretty trivial. When extracting a wheel, do not recheck/recreate the parent directory before extracting each file.

Profiling a run of pip install plotly --prefix ./testdir

PROFILER ON MASTER:
image

image

PROFILER WITH THIS FIX:
image

image

Notice the 14425 calls to ensure_dir() taking 512ms total or ~5% of the runtime.
Notice it's a lot less with the fix.

Total runtime 10.268 -> 9.414 seconds, thanks to all the calls removed + a few hundreds of ms of variance between pip runs.

Regards.

@notatallshaw
Copy link
Member

FYI, the Windows CI failings are unrelated to your PR

@ichard26 ichard26 added the type: performance Commands take too long to run label Jun 22, 2024
@ichard26
Copy link
Member

ichard26 commented Jun 22, 2024

Hi, just a heads up that we just cut pip v24.1 so the core team's resources are being focused on making sure that release is handled smoothly. This PR looks simple so I'll try to take look tomorrow, but in the meanwhile, could you add a NEWS entry? Thank you!

@morotti
Copy link
Contributor Author

morotti commented Jun 24, 2024

Thanks, I added a NEWS entry.

@morotti
Copy link
Contributor Author

morotti commented Jun 24, 2024

Builds is failing after rerunning, there are issues with windows builds and 3.13?
trying to pickup changes on master for windows but then it's failing even earlier with new ruff rules?

src/pip/_internal/operations/install/wheel.py:418:5: C901 `_install_wheel` is too complex (34 > 33)
src/pip/_internal/operations/install/wheel.py:418:5: PLR0915 Too many statements (139 > 134)

@morotti
Copy link
Contributor Author

morotti commented Jun 26, 2024

All green, should be good to merge now.

Copy link
Member

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

This modifies the contract between ZipBackedFile.save() and its caller, but it seems fine. Perhaps a comment explaining that this logic has been moved out for performance reasons would be helpful.

Thanks again for the PR!

news/12782.bugfix.rst Outdated Show resolved Hide resolved
@pradyunsg pradyunsg changed the title PERF: 5% faster pip install, when extracting a wheel, do not recheck/rec… When extracting a wheel, do not recheck parent directories Jun 27, 2024
@morotti
Copy link
Contributor Author

morotti commented Jul 4, 2024

this PR is ready to merge

@ichard26
Copy link
Member

ichard26 commented Jul 6, 2024

I'm going to schedule this PR for 24.2 as it seems like a small low-risk performance tweak. Feel free to remove the PR from the milestone if you object.

@ichard26 ichard26 added this to the 24.2 milestone Jul 6, 2024
@pradyunsg pradyunsg merged commit 1dbaf48 into pypa:main Jul 9, 2024
28 checks passed
@pradyunsg
Copy link
Member

Thanks @morotti! ^.^

@morotti
Copy link
Contributor Author

morotti commented Jul 10, 2024

Thank you very much @pradyunsg

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants