-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
gh-117641: Use set comprehension for posixpath.commonpath()
#117652
gh-117641: Use set comprehension for posixpath.commonpath()
#117652
Conversation
@serhiy-storchaka, do you have suggestions here? |
I understand the use of set comprehension instead of generator, it can add a tiny speed up. But why get rid of unpack? |
I wanted to make the code more readable. But I'll revert it as it's slower: def test1(paths):
sep = '/'
try:
isabs, = {p.startswith(sep) for p in paths}
except ValueError:
raise ValueError("Can't mix absolute and relative paths") from None
prefix = sep if isabs else sep[:0]
return prefix
def test2(paths):
sep = '/'
if len({p.startswith(sep) for p in paths}) != 1:
raise ValueError("Can't mix absolute and relative paths")
prefix = sep if paths[0].startswith(sep) else sep[:0]
return prefix 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 # unpack
500000 loops, best of 5: 451 nsec per loop # no unpack
# -> 1.22x slower |
@erlend-aasland, do you think this can be merged now? I addressed serhiy-storchaka's question. |
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. |
@erlend-aasland, could you merge it? It has been approved by serhiy. |
@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. |
Thanks.
Sorry, sometimes I get no response, and then I don't know if my message was even read. |
Benchmark
script
posixpath.commonpath()
#117641