Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-100562: improve performance of pathlib.Path.absolute()#100563
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
gh-100562: improve performance of pathlib.Path.absolute()#100563
Uh oh!
There was an error while loading. Please reload this page.
Conversation
barneygale commented Dec 28, 2022 • edited by miss-islington
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by miss-islington
Uh oh!
There was an error while loading. Please reload this page.
Increase performance of the `absolute()` method by calling `os.getcwd()` directly, rather than using the `Path.cwd()` class method. This avoids constructing an extra `Path` object (and the parsing/normalization that comes with it). Decrease performance of the `cwd()` class method by calling the `Path.absolute()` method, rather than using `os.getcwd()` directly. This involves constructing an extra `Path` object. We do this to maintain a longstanding pattern where `os` functions are called from only one place, which allows them to be more readily replaced by users. As `cwd()` is generally called at most once within user programs, it's a good bargain.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
AlexWaygood left a comment
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 looks great to me. I agree that it's much more important to optimise Path.absolute() than Path.cwd(), and I can reproduce the huge speedup.
Is the plan still to hold off merging this for now until #100563 (comment) is resolved, or can this be merged at any time?
Misc/NEWS.d/next/Library/2022-12-28-00-28-43.gh-issue-100562.Hic0Z0.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
barneygale commented Jan 5, 2023
Thanks for the review!
I think this can be merged any time. The backport for the fix for Eryk's issue will be slightly more complicated, but I will cope :D |
…ic0Z0.rst Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood commented Jan 5, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I think all the tests are failing because Guido just broke the But I'll re-run the tests here as soon as the fix in #100778 is merged (just to be on the safe side), then I'll merge! |
miss-islington commented Jan 5, 2023
Status check is done, and it's a success ✅. |
bedevere-bot commented Jan 5, 2023
|
AlexWaygood commented Jan 5, 2023
Looks like a fluke to me, I can't see how this change would mean that |
Increase performance of the
absolute()method by callingos.getcwd()directly, rather than using thePath.cwd()class method. This avoids constructing an extraPathobject (and the parsing/normalization that comes with it).Decrease performance of the
cwd()class method by calling thePath.absolute()method, rather than usingos.getcwd()directly. This involves constructing an extraPathobject. We do this to maintain a longstanding pattern whereosfunctions are called from only one place, which allows them to be more readily replaced by users. Ascwd()is generally called at most once within user programs, it's a good bargain.Automerge-Triggered-By: GH:AlexWaygood