GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-117587: Add C implementation of `os.path.abspath` by nineteendo · Pull Request #117855 · python/cpython

Conversation

nineteendo commented Apr 13, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reviewed

Apr 13, 2024

View reviewed changes

Comment thread

Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reviewed

Apr 13, 2024

View reviewed changes

Comment thread

Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Modules/posixmodule.c

cc3a59c

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

eendebakpt reviewed

Apr 13, 2024

View reviewed changes

Comment thread

Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

eendebakpt reviewed

Apr 13, 2024

View reviewed changes

Comment thread

Python/fileutils.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

eendebakpt reviewed

Apr 13, 2024

View reviewed changes

Comment thread

Python/fileutils.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Revert changes to os__path_normpath_impl

ae2c040

nineteendo commented

Apr 14, 2024

View reviewed changes

Comment thread

Lib/posixpath.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          simply os.fsdecode(os.getcwdb())

95773d9

This comment was marked as resolved.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Just check if path is absolute

1ebe5f4

erlend-aasland requested changes

May 30, 2024

View reviewed changes

Comment thread

Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot added awaiting changes and removed awaiting change review labels

May 30, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Narrow scope of cwd_buf

e047f03

nineteendo commented May 30, 2024

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot added awaiting change review and removed awaiting changes labels

May 30, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 Resolved

Uh oh!

There was an error while loading. Please reload this page.

nineteendo commented

May 30, 2024

View reviewed changes

Comment thread

Python/fileutils.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

picnixz left a comment

Copy link

Copy Markdown

Member

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.

Some nitpicks

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/fileutils.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/fileutils.c Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Apply suggestions from code review

54c7021

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

erlend-aasland reviewed

May 30, 2024

View reviewed changes

Comment thread

Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added the DO-NOT-MERGE label

May 30, 2024


          Use explicit return values

dc66ef9

eryksun reviewed

May 30, 2024

View reviewed changes

Comment thread

Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nineteendo commented Apr 13, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.