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

gh-117394: Reduce syscalls for `posixpath.ismount` by nineteendo · Pull Request #117395 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-117394: Reduce syscalls for posixpath.ismount - #117395

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

gh-117394: Reduce syscalls for posixpath.ismount#117395
nineteendo wants to merge 5 commits into
python:mainfrom
nineteendo:speedup-posixpath.ismount

Conversation

nineteendo commented Mar 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Benchmark:

python -m timeit -s "import before.posixpath" "before.posixpath.ismount('/Volumes/2GB_001')" && python -m timeit -s "import after.posixpath" "after.posixpath.ismount('/Volumes/2GB_001')"
10000 loops, best of 5: 21.3 usec per loop # before
50000 loops, best of 5: 8.5 usec per loop # after
# -> 2.51x faster

bedevere-app Bot mentioned this pull request Mar 30, 2024
Closed
AlexWaygood requested a review from barneygale March 30, 2024 19:39
Update Misc/NEWS.d/next/Core and Builtins/2024-03-30-19-34-51.gh-issu…
cb532fc 
…e-117394.qCepkD.rst

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
nineteendo marked this pull request as ready for review March 30, 2024 19:42

barneygale commented Mar 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

What happens if /mnt/storage is a mount point, /mnt/storage/link is a symlink to /mnt, and you call ismount('/mnt/storage/link/storage')? I think the new implementation gives a different answer.

nineteendo mentioned this pull request Mar 30, 2024
Closed
16 tasks done

nineteendo commented Apr 1, 2024

Copy link
Copy Markdown
Contributor Author

Good thinking! The old implementation returns True in that case, the new implementation False.
But that can easily fixed by replacing os.lstat() with os.stat().

barneygale commented Apr 1, 2024

Copy link
Copy Markdown
Contributor

Serhiy suggested a similar patch previously: #46718 (comment)

Compared to Serhiy's patch, this PR adds a abspath() call, which internally calls normpath(). I think that eliminates the remaining edge cases (e.g. trailing slashes, dots in paths, etc).

nineteendo commented Apr 1, 2024

Copy link
Copy Markdown
Contributor Author

What's going on here? https://github.com/python/cpython/actions/runs/8504085005/job/23290509561?pr=117395#step:16:481
I don't know the value of ABSTFN.

nineteendo commented Apr 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Oh, I know why. This test needs to be updated: it monkey patches os.lstat instead of os.stat.

serhiy-storchaka commented Apr 2, 2024

Copy link
Copy Markdown
Member

Sorry, but abspath() is unsuitable for this. See details in #117394 (comment).

nineteendo deleted the speedup-posixpath.ismount branch April 3, 2024 14:25
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

AlexWaygood AlexWaygood left review comments
barneygale Awaiting requested review from barneygale
serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL