Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-117641: Use set comprehension forposixpath.commonpath()#117652
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-117641: Use set comprehension forposixpath.commonpath()#117652
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
nineteendo commentedApr 13, 2024
@serhiy-storchaka, do you have suggestions here? |
serhiy-storchaka commentedApr 15, 2024
I understand the use of set comprehension instead of generator, it can add a tiny speed up. But why get rid of unpack? |
nineteendo commentedApr 16, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I wanted to make the code more readable. But I'll revert it as it's slower: deftest1(paths):sep='/'try:isabs,= {p.startswith(sep)forpinpaths}exceptValueError:raiseValueError("Can't mix absolute and relative paths")fromNoneprefix=sepifisabselsesep[:0]returnprefixdeftest2(paths):sep='/'iflen({p.startswith(sep)forpinpaths})!=1:raiseValueError("Can't mix absolute and relative paths")prefix=sepifpaths[0].startswith(sep)elsesep[:0]returnprefix wannes@Stefans-iMac Desktop % python -m timeit -s"import test""test.test1(['foo'])"&& python -m timeit -s"import test""test.test2(['foo'])"1000000 loops, best of 5: 369 nsec per loop# unpack500000 loops, best of 5: 451 nsec per loop# no unpack# -> 1.22x slower |
nineteendo commentedApr 17, 2024
@erlend-aasland, do you think this can be merged now? I addressed serhiy-storchaka's question. |
erlend-aasland commentedApr 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'll run PGO benchmarks for the updated PR first. UPDATE: I'm getting between 5% and 20% speedups, similar to the speedup in the PR description. |
nineteendo commentedApr 17, 2024
@erlend-aasland, could you merge it? It has been approved by serhiy. |
erlend-aasland commentedApr 17, 2024
@nineteendo: yes, it was on my list; there is no need for the extra ping. There is no hurry; I'll land it tomorrow morning. Please avoid unneeded pings. |
nineteendo commentedApr 17, 2024
Thanks.
Sorry, sometimes I get no response, and then I don't know if my message was even read. |
Uh oh!
There was an error while loading.Please reload this page.
Benchmark
script
posixpath.commonpath()#117641