Skip to content

Conversation

nineteendo
Copy link
Contributor

@nineteendo nineteendo commented Oct 8, 2024

Originally proposed in #120146.

Benchmark

Show script
# patch-1.sh
mkdir -p "tmp/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a"

echo 200 .
main/python.exe    -m timeit -s "import os; path = './' * 200"   "os.path.realpath(path)"
patch-1/python.exe -m timeit -s "import os; path = './' * 200"   "os.path.realpath(path)"

echo 200 ..
main/python.exe    -m timeit -s "import os; path = '../' * 200"   "os.path.realpath(path)"
patch-1/python.exe -m timeit -s "import os; path = '../' * 200"   "os.path.realpath(path)"

echo 200 dirs
main/python.exe    -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/a' * 200" "os.path.realpath(path)"
patch-1/python.exe -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/a' * 200" "os.path.realpath(path)"
200 . - no difference
10000 loops, best of 5: 26.9 usec per loop
10000 loops, best of 5: 26.7 usec per loop
200 .. - 1.02x faster
5000 loops, best of 5: 50.3 usec per loop
5000 loops, best of 5: 49.2 usec per loop
200 dirs - no difference
100 loops, best of 5: 2.19 msec per loop
100 loops, best of 5: 2.16 msec per loop
$ python -m timeit -s "name = '.'" "pass" "if name is None: pass"
50000000 loops, best of 5: 9.03 nsec per loop
$ python -m timeit -s "name = '.'" "pass"
50000000 loops, best of 5: 6.08 nsec per loop

@nineteendo nineteendo changed the title Speed up posixpath.realpath() in the common case gh-125159: Speed up posixpath.realpath() in the common case Oct 8, 2024
@nineteendo
Copy link
Contributor Author

cc @barneygale

@ericvsmith
Copy link
Member

Does this make a measurable difference?

@Wulian233
Copy link
Contributor

Could you provide the results of the speed test?

Co-authored-by: Barney Gale <[email protected]>
@nineteendo
Copy link
Contributor Author

It's not a lot faster, 2.95 nsec/iteration.

@ericvsmith
Copy link
Member

I think this change isn't worth the churn.

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Oct 10, 2024
@nineteendo
Copy link
Contributor Author

Just as expected.

@nineteendo nineteendo closed this Oct 11, 2024
@nineteendo nineteendo deleted the patch-1 branch October 11, 2024 05:56
@erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label Oct 11, 2024
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Oct 11, 2024

Just as expected.

If I understand you correctly, you expected this PR to be rejected.

The next time you consider opening a PR that you "expect to be rejected", please consider again the following:

  • is it worth the reviewer and CI churn to post a "ready-to-be-rejected" PR?
  • is there anything you can improve pre proposing the PR that can increase the chances of it being accepted? (benchmarks, seeking consensus about the change, backwards compatibility, etc.) See also the devguide: Making good PRs

@barneygale
Copy link
Contributor

@erlend-aasland it's my fault - see #120146 (comment)

@nineteendo really sorry for giving you bad advice and wasting your time

@nineteendo
Copy link
Contributor Author

Thanks for being honest, Barney, but it's also my fault. I knew it wasn't a lot faster (much less than the 5% Alex mentioned in the past) and therefore probably wasn't worth a PR to change just that. That doesn't mean that making the code 3 nanoseconds slower for speeding up a rare case is fine though.

@vstinner
Copy link
Member

In general, an optimization is worth it if it's at least 10% faster on a microbenchmark. It doesn't seem to be the case here.

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.

6 participants