-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-128118: Improve performance of copy.copy by using a fast lookup for atomic and container types #128119
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
Conversation
Misc/NEWS.d/next/Library/2024-12-20-10-57-10.gh-issue-128118.mYak8i.rst
Outdated
Show resolved
Hide resolved
def _copy_immutable(x): | ||
return x | ||
for t in (types.NoneType, int, float, bool, complex, str, tuple, | ||
_copy_atomic_types = {types.NoneType, int, float, bool, complex, str, tuple, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, would performance be better if we use a frozenset instead of a set? (and is it possible?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I'll benchmark a bit later. A frozenset should not require any locking, so perhaps there is a difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this moment the set
and frozenset
have the same implementation for __contains__
:
Lines 2529 to 2531 in 3bd7730
static PyMethodDef frozenset_methods[] = { | |
SET___CONTAINS___METHODDEF | |
FROZENSET_COPY_METHODDEF |
Lines 2416 to 2420 in 3bd7730
static PyMethodDef set_methods[] = { | |
SET_ADD_METHODDEF | |
SET_CLEAR_METHODDEF | |
SET___CONTAINS___METHODDEF | |
SET_COPY_METHODDEF |
so there is no performance difference. In the future however, for the free-threading build one could remove the critical section for the frozenset implementation here:
Lines 2198 to 2207 in 3bd7730
static int | |
set_contains(PyObject *self, PyObject *key) | |
{ | |
PySetObject *so = _PySet_CAST(self); | |
return _PySet_Contains(so, key); | |
} | |
/*[clinic input] | |
@critical_section | |
@coexist |
Using a frozenset is possible, but this would add a bit of time to the import. On my system %timeit frozenset(_copy_atomic_types)
is about 300 ns
Even faster than a set
would be a data structure that looks only at the id of the objects involved (the set
will use rich compare if no match is found, but that is not needed as all objects involved are singletons), but that is not available in cpython I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but that is not available in cpython I believe.
That's right, it's not available.
Up to you if you want to make the free-threaded build faster in the future, but we should probably check the performances on this build. For now, let's keep the set for now (hopefully you'll rememeber this)
…Yak8i.rst Co-authored-by: Bénédikt Tran <[email protected]>
Thanks for the speed-up, Pieter! Thanks for the reviews, Bénédikt and Sergey! |
Similar to the approached used for
copy.deepcopy
in #114266 we can simplifly the implementation ofcopy.copy
and improve performance by checking on the type of the argument using a lookup.Results:
Benchmark script: