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

gh-117587: Add C implementation of `os.path.abspath` by nineteendo · Pull Request #117855 · python/cpython · GitHub

/ cpython Public

gh-117587: Add C implementation of os.path.abspath - #117855

Closed
nineteendo wants to merge 87 commits into
python:mainfrom
nineteendo:speedup-posixpath.abspath
ConversationCommits (87)ChecksFiles changed
Closed

gh-117587: Add C implementation of os.path.abspath#117855
nineteendo wants to merge 87 commits into
python:mainfrom
nineteendo:speedup-posixpath.abspath

Conversation

nineteendo commented Apr 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Benchmark

posixpath.py by @eryksun:

absolute, with "/a" repeated
len   speedup
100   1.521x
500   1.157x
1000  1.128x
2000  1.068x
4000  1.019x

relative 'a', with cwd length
len     speedup
93+2    1.861x
548+2   1.729x
1003+2  1.881x
2043+2  1.556x
4058+2  1.548x

ntpath.py

script 
::speedup-posixpath.abspath.bat
@echo off
echo 10 chars && call main\python -m timeit -s "import os" "os.path.abspath('a/' * 5)" && call speedup-posixpath.abspath\python -m timeit -s "import os" "os.path.abspath('a/' * 5)"
echo 100 chars && call main\python -m timeit -s "import os" "os.path.abspath('a/' * 50)" && call speedup-posixpath.abspath\python -m timeit -s "import os" "os.path.abspath('a/' * 50)"
echo 1000 chars && call main\python -m timeit -s "import os" "os.path.abspath('a/' * 500)" && call speedup-posixpath.abspath\python -m timeit -s "import os" "os.path.abspath('a/' * 500)"
10 chars
500000 loops, best of 5: 635 nsec per loop # before
500000 loops, best of 5: 517 nsec per loop # after
# -> 1.23x faster
100 chars
200000 loops, best of 5: 1.53 usec per loop # before
200000 loops, best of 5: 1.23 usec per loop # after
# -> 1.24x faster
1000 chars
50000 loops, best of 5: 9.87 usec per loop # before
50000 loops, best of 5: 7.73 usec per loop # after
# -> 1.28x faster

bedevere-app Bot mentioned this pull request Apr 13, 2024
Closed
nineteendo marked this pull request as ready for review April 13, 2024 20:03
erlend-aasland requested a review from eryksun April 13, 2024 20:35
erlend-aasland changed the title gh-117587: Speedup posixpath.abspath gh-117587: Add C implementation of posixpath.abspath Apr 13, 2024
eendebakpt reviewed Apr 13, 2024
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
erlend-aasland reviewed Apr 13, 2024
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
erlend-aasland reviewed Apr 13, 2024
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
Update Modules/posixmodule.c
cc3a59c 
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
eendebakpt reviewed Apr 13, 2024
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
eendebakpt reviewed Apr 13, 2024
View reviewed changes
Comment thread Python/fileutils.c Outdated
eendebakpt reviewed Apr 13, 2024
View reviewed changes
Comment thread Python/fileutils.c
nineteendo and others added 2 commits April 13, 2024 23:43
Update Python/fileutils.c
f5f2c7b 
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
fastcall & unicode type
a1bc125 
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland reviewed Apr 13, 2024
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
nineteendo commented Apr 14, 2024
View reviewed changes
Comment thread Lib/posixpath.py Outdated

nineteendo commented Apr 14, 2024

Copy link
Copy Markdown
Contributor Author

The same approach doesn't work on Window, because ntpath.join() is much slower than nt._getfullpathname():

PS C:\Users\wanne\cpython> python -m timeit -s "import nt" "nt._getfullpathname('.')"; python -m timeit -s "import os" "os.path.join(r'C:\Users\wanne\cpython', '')"
1000000 loops, best of 5: 286 nsec per loop # _getfullpathname
200000 loops, best of 5: 1.41 usec per loop # join

This comment was marked as resolved.

Sign in to view

nineteendo commented Apr 14, 2024

Copy link
Copy Markdown
Contributor Author

Preferably, abspath() should not be naively implemented by simply joining a relative path with the working directory. It doesn't work for drive-relative paths. That's a long-standing bug in the generic implementation ntpath._abspath_fallback().

I didn't even get that far in the testing process. I already knew it wasn't going to work.
I did notice this while trying to eliminate normpath() from the current implementation.

bedevere-app Bot commented May 29, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

@erlend-aasland: please review the changes made to this pull request.

bedevere-app Bot requested a review from erlend-aasland May 29, 2024 21:23
nineteendo commented May 29, 2024
View reviewed changes
Comment thread Lib/test/test_ntpath.py Outdated
erlend-aasland requested changes May 30, 2024
View reviewed changes
Comment thread Modules/posixmodule.c Outdated

bedevere-app Bot commented May 30, 2024

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.

nineteendo commented May 30, 2024

Copy link
Copy Markdown
Contributor Author

erlend-aasland, I have made the requested changes; please review again.

bedevere-app Bot commented May 30, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

@erlend-aasland: please review the changes made to this pull request.

bedevere-app Bot requested a review from erlend-aasland May 30, 2024 12:22
erlend-aasland reviewed May 30, 2024
View reviewed changes
Comment thread Modules/posixmodule.c
nineteendo commented May 30, 2024
View reviewed changes
Comment thread Python/fileutils.c Outdated
picnixz reviewed May 30, 2024
View reviewed changes

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

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

Some nitpicks

Comment thread Python/fileutils.c Outdated
Comment thread Python/fileutils.c Outdated
Apply suggestions from code review
54c7021 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
erlend-aasland reviewed May 30, 2024
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
eryksun reviewed May 30, 2024
View reviewed changes
Comment thread Modules/posixmodule.c
nineteendo deleted the speedup-posixpath.abspath branch May 31, 2024 06:45
This was referenced Nov 13, 2024
Closed
Open
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

eendebakpt eendebakpt left review comments
eryksun eryksun approved these changes
barneygale barneygale left review comments
picnixz picnixz left review comments
erlend-aasland erlend-aasland requested changes

Assignees

No one assigned

Labels

awaiting change review DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL