I believe that's everything. If you would like these changes to be split up in multiple pull requests let me know.

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

Please revert all changes that do not actually have any impact on performance.

Does that include unnesting?

Is there anything you still expect from me? Or can this finally be merged? I've listened to basically all the feedback.
If you want to wait for a benchmark from me, that's understandable of course.

The speed improvements seem way less impressive now. ;(
And I that's even without running my benchmark.

Are you happy now?

I re-ran your benchmarks locally. With the latest version of your PR, I'm getting a reasonableslowdown onposixpath.ismount() from your PR branch.

Onmain:

(main) % ./python.exe -m timeit -s "import posixpath" "posixpath.ismount('/Volumes/2GB_001')"                                 ~/dev/cpython500000 loops, best of 5: 602 nsec per loop(main) % ./python.exe -m timeit -s "import posixpath" "posixpath.ismount('/Volumes/2GB_001')"                                 ~/dev/cpython500000 loops, best of 5: 603 nsec per loop(main) % ./python.exe -m timeit -s "import posixpath" "posixpath.ismount('/Volumes/2GB_001')"                                 ~/dev/cpython500000 loops, best of 5: 607 nsec per loop(main) % ./python.exe -m timeit -s "import posixpath" "posixpath.ismount('/Volumes/2GB_001')"                                 ~/dev/cpython500000 loops, best of 5: 601 nsec per loop

With your PR branch:

(speedup-os.path) % ./python.exe -m timeit -s "import posixpath" "posixpath.ismount('/Volumes/2GB_001')"                      ~/dev/cpython500000 loops, best of 5: 637 nsec per loop(speedup-os.path) % ./python.exe -m timeit -s "import posixpath" "posixpath.ismount('/Volumes/2GB_001')"                      ~/dev/cpython500000 loops, best of 5: 650 nsec per loop(speedup-os.path) % ./python.exe -m timeit -s "import posixpath" "posixpath.ismount('/Volumes/2GB_001')"                      ~/dev/cpython500000 loops, best of 5: 646 nsec per loop(speedup-os.path) % ./python.exe -m timeit -s "import posixpath" "posixpath.ismount('/Volumes/2GB_001')"                      ~/dev/cpython500000 loops, best of 5: 643 nsec per loop

With the latest version of your PR, I'm getting a reasonable slowdown on posixpath.ismount() from your PR branch.

You don't have a usb stick "2GB_001" plugged in, so the function will returnFalse immediately afters1 = os.lstat(path).
I believe something has been sped up on the main branch, so I just synced my branch.

You don't have a usb stick "2GB_001" plugged in, so the function will returnFalse immediately afters1 = os.lstat(path).

heh, that makes sense

I've updated the benchmarks. Sadly, the only noticeable speedup for regular users is callingposixpath.abspath() with an absolute path.

I've updated the benchmarks. Sadly, the only noticeable speedup for regular users is callingposixpath.abspath() with an absolute path.

Don't be too disheartened. As a result of your efforts here, a conversation has been started about possible ways of optimisingstr.startswith and other string methods. If one or more of those ideas comes to fruition, that will bevery impactful for Python users.

Optimising the stdlib is hard! I tried outmany things that ultimately didn't work when I was working on#74690. And there were several things thatdid work, but which I never created PRs for, as they would have made the code too ugly or too fragile.

My advice for the future, however, would definitely be to work on small, focused PRs that have isolated, easily measurable changes. PRs like that are much easier for us to review, and you should find the contributing experience less frustrating as a result.

Can this be merged, or are we still waiting on something?

Can this be merged, or are we still waiting on something?

It can be merged, I just wanted to wait a little bit longer to give the other reviewers time to chime in on the final version of the PR, if they wanted to. I'll merge tomorrow or the day after if there are no further objections from anybody.

Thanks@nineteendo

I believe it should indeed be possible to speed upstr.startswith(prefix), but notstr.startswith((prefix1, prefix2)), because there's not guarantee that the prefixes have the same length. I'll revert the optimisations for the first case.

Both cases were significantly improved by#117466:

# pre optimisation$ ./python.exe -m timeit -s "s = 'abcdef'" "s.startswith(('abc', 'de'))"5000000 loops, best of 5: 89 nsec per loop# post optimisation$ ./python.exe -m timeit -s "s = 'abcdef'" "s.startswith(('abc', 'de'))"10000000 loops, best of 5: 26.7 nsec per loop

I think we should be careful about micro-optimising Python code, like this PR. Instead, it would be better to keep the code as idiomatic as possible, and instead optimise the Python interpreter for the idiomatic cases.

I think we should be careful about micro-optimising Python code, like this PR. Instead, it would be better to keep the code as idiomatic as possible, and instead optimise the Python interpreter for the idiomatic cases.

I agree that we should neither accept micro-optimisations that make code significantly less idiomatic, nor changes that are purely cosmetic and have no impact on performance. I accepted the PR nonetheless because in the final iteration of the PR, all changes seemed to me to be small, localised changes that, as well as providing small speedups, mostly made the code more idiomatic, and in all cases (in my opinion) did not cause a significant deterioration in style.

Both cases were significantly improved by#117466

I didn't expect the second case to be sped up as it was a sequence, so I asked for clarification:faster-cpython/ideas#671 (comment), but didn't get a response.

This speedup must have been added afterwards, which I missed.
I obviously reverted the regularstr.startswith() &str.endswith().