-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
Add fast path to deepcopy()
for empty list/tuple/dict/set
#121192
Comments
@lgeiger No worries about conflicts with my PR, I can rebase if needed. I will add benchmarks for empty containers as well (I never imagined they would actually matter). More important is perhaps whether your patch slows down the other cases. Do you have numbers for that? And perhaps also a benchmark for the pydantic models. |
I could reduce the slowdown of the default case to 1.06x with a slightly simpler version of the patch that actually performs even better for empty lists, sets and dicts but is a bit slower for tuples and frozen sets (although still much faster than the baseline). Let me know if you'd like me to open a PR, maybe then it's easier to discuss with the concrete code.
|
Why check |
Good idea, changed in ba7298c. This makes it slightly faster as well. In my quick benchmark the PR would make the default case only 1.03x slower (see updated numbers in the PR) |
I think that from 4x to 20x speed up for this quite common case is a good enough reason to slow down for 1.09x in the slow path. I am +1 on this. |
@sobolevn Out of curiosity, what is the typical case for deep copy? Deep cooy for non empty containers or Deepcopy for empty containers? I think the slowdown is not cheap if the nonempty container is more frequently called in the real world. |
deepcopy()
can be surprisingly slow when called with empty containers like lists, tuples, dicts, sets or frozensets.Adding a fast path for this case similar to #114266 would significantly speed up such cases by about 4x - 28x while having little impact on the default path and not adding too much complexity. With such a patch the following benchmarking script would show a significant speedup compared to
main
:This might conflict with @eendebakpt efforts in #91610 or could be something that should be added to the proposed C version as well.
For context, I noticed this when using pydantic models with mutable default values where pydantic would deep copy the default value upon class instantiation. E.g.:
To be fair the proper fix in this case would be not to use a mutable default value in pydantic and switch to
pydantic.Field(default_factory=list)
similar to dataclasses instead which is much faster. But potentially there might be other scenarios where deepcopying empty iterables might be common.I'm happy to make a PR unless it conflicts with the efforts going on in #91610.
Linked PRs
deepcopy()
for empty list/tuple/dict/set #121193The text was updated successfully, but these errors were encountered: