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

nineteendo · 2024-04-13T17:58:49Z

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

Issue: Speedup posixpath.abspath() for relative paths #117587

Modules/posixmodule.c

Co-authored-by: Pieter Eendebak <[email protected]>

Modules/posixmodule.c

Python/fileutils.c

Co-authored-by: Pieter Eendebak <[email protected]>

Co-authored-by: Pieter Eendebak <[email protected]> Co-authored-by: Erlend E. Aasland <[email protected]>

Modules/posixmodule.c

Lib/posixpath.py

nineteendo · 2024-04-14T15:44:23Z

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

nineteendo · 2024-04-14T17:52:37Z

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.

…endo/cpython into speedup-posixpath.abspath

Co-authored-by: Erlend E. Aasland <[email protected]>

bedevere-app · 2024-05-29T21:23:14Z

Thanks for making the requested changes!

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

Lib/test/test_ntpath.py

Modules/posixmodule.c

bedevere-app · 2024-05-30T09:20:45Z

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 · 2024-05-30T12:22:47Z

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

bedevere-app · 2024-05-30T12:22:53Z

Thanks for making the requested changes!

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

Modules/posixmodule.c

Python/fileutils.c

picnixz

Some nitpicks

Python/fileutils.c

Co-authored-by: Bénédikt Tran <[email protected]>

Modules/posixmodule.c

nineteendo added 3 commits April 12, 2024 16:15

Speedup posixpath.abspath() for relative paths

2475af9

Fix cwd length for byte paths & bus error

807b494

Merge branch 'python:main' into speedup-posixpath.abspath

aa3ed0c

bedevere-app bot mentioned this pull request Apr 13, 2024

Speedup posixpath.abspath() for relative paths #117587

Closed

blurb-it bot and others added 3 commits April 13, 2024 18:00

📜🤖 Added by blurb_it.

fa50a4c

Merge branch 'main' into speedup-posixpath.abspath

f502dd1

Fix _Py_normpath_and_size when size=-1

716f594

nineteendo marked this pull request as ready for review April 13, 2024 20:03

bedevere-app bot added the awaiting review label Apr 13, 2024

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

Modules/posixmodule.c Outdated Show resolved Hide resolved

erlend-aasland reviewed Apr 13, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

erlend-aasland reviewed Apr 13, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

Update Modules/posixmodule.c

cc3a59c

Co-authored-by: Pieter Eendebak <[email protected]>

eendebakpt reviewed Apr 13, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

eendebakpt reviewed Apr 13, 2024

View reviewed changes

Python/fileutils.c Outdated Show resolved Hide resolved

eendebakpt reviewed Apr 13, 2024

View reviewed changes

Python/fileutils.c Show resolved Hide resolved

nineteendo and others added 2 commits April 13, 2024 23:43

Update Python/fileutils.c

f5f2c7b

Co-authored-by: Pieter Eendebak <[email protected]>

fastcall & unicode type

a1bc125

Co-authored-by: Pieter Eendebak <[email protected]> Co-authored-by: Erlend E. Aasland <[email protected]>

erlend-aasland reviewed Apr 13, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

Revert changes to os__path_normpath_impl

ae2c040

nineteendo commented Apr 14, 2024

View reviewed changes

Lib/posixpath.py Outdated Show resolved Hide resolved

simply os.fsdecode(os.getcwdb())

95773d9

This comment was marked as resolved.

Sign in to view

nineteendo and others added 3 commits April 14, 2024 21:25

1st attempt at full C implementation

b77a15d

Merge branch 'speedup-posixpath.abspath' of https://github.com/ninete…

f39ab83

…endo/cpython into speedup-posixpath.abspath

Update Modules/posixmodule.c

c63518b

Co-authored-by: Erlend E. Aasland <[email protected]>

bedevere-app bot added awaiting change review and removed awaiting changes labels May 29, 2024

bedevere-app bot requested a review from erlend-aasland May 29, 2024 21:23

nineteendo commented May 29, 2024

View reviewed changes

Lib/test/test_ntpath.py Outdated Show resolved Hide resolved

Just check if path is absolute

1ebe5f4

erlend-aasland requested changes May 30, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

bedevere-app bot added awaiting changes and removed awaiting change review labels May 30, 2024

Narrow scope of cwd_buf

e047f03

bedevere-app bot added awaiting change review and removed awaiting changes labels May 30, 2024

bedevere-app bot requested a review from erlend-aasland May 30, 2024 12:22

erlend-aasland reviewed May 30, 2024

View reviewed changes

Modules/posixmodule.c Show resolved Hide resolved

nineteendo commented May 30, 2024

View reviewed changes

Python/fileutils.c Outdated Show resolved Hide resolved

nineteendo added 2 commits May 30, 2024 14:43

Update Python/fileutils.c

625fc79

Free memory before calling the PyErr APIs

fc3f227

picnixz reviewed May 30, 2024

View reviewed changes

Python/fileutils.c Outdated Show resolved Hide resolved

Python/fileutils.c Outdated Show resolved Hide resolved

Apply suggestions from code review

54c7021

Co-authored-by: Bénédikt Tran <[email protected]>

erlend-aasland reviewed May 30, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

erlend-aasland added the DO-NOT-MERGE label May 30, 2024

Use explicit return values

dc66ef9

eryksun reviewed May 30, 2024

View reviewed changes

Modules/posixmodule.c Show resolved Hide resolved

erlend-aasland closed this May 30, 2024

nineteendo deleted the speedup-posixpath.abspath branch May 31, 2024 06:45

This was referenced Nov 13, 2024

Fix ntpath.normpath() for drive-relative paths #126780

Closed

Support qualified referencing for ntpath.abspath() #126782

Open

Uh oh!

Comments

Conversation

nineteendo commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark

posixpath.py by @eryksun:

ntpath.py

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nineteendo commented Apr 14, 2024

Uh oh!

This comment was marked as resolved.

nineteendo commented Apr 14, 2024

Uh oh!

bedevere-app bot commented May 29, 2024

Uh oh!

Uh oh!

Uh oh!

bedevere-app bot commented May 30, 2024

Uh oh!

nineteendo commented May 30, 2024

Uh oh!

bedevere-app bot commented May 30, 2024

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nineteendo commented Apr 13, 2024 •

edited

Loading