Skip to content

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

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

Closed
wants to merge 5 commits into from

Conversation

nineteendo
Copy link
Contributor

@nineteendo nineteendo commented Mar 30, 2024

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

@AlexWaygood AlexWaygood requested a review from barneygale March 30, 2024 19:39
@nineteendo nineteendo marked this pull request as ready for review March 30, 2024 19:42
@barneygale
Copy link
Contributor

barneygale commented Mar 30, 2024

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 nineteendo mentioned this pull request Mar 30, 2024
16 tasks
@nineteendo
Copy link
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
Copy link
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
Copy link
Contributor Author

@nineteendo
Copy link
Contributor Author

nineteendo commented Apr 1, 2024

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

@serhiy-storchaka
Copy link
Member

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

@nineteendo nineteendo deleted the speedup-posixpath.ismount branch April 3, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants