gh-154594: Fix copy.deepcopy() re-copying objects whose __deepcopy__ returns None - #154819
gh-154594: Fix copy.deepcopy() re-copying objects whose __deepcopy__ returns None#154819sreehariannam wants to merge 2 commits into
Conversation
…opy__ returns None deepcopy() used None both as the memo dict's "not cached" sentinel and as a value callers can legitimately store there, so a __deepcopy__ that returns None was indistinguishable from a cache miss and got invoked again on every subsequent reference to the same object. Use a direct memo[d] lookup (matching the existing pattern in _deepcopy_tuple/_deepcopy_frozendict) instead of memo.get(d, None), so every return value -- including None -- is memoized correctly. This also avoids reintroducing the non-immortal-sentinel refcount contention under free-threading that pythongh-132657/pythongh-138429 fixed, since no sentinel object is used at all.
|
@sreehariannam Can you create benchmarks for your approach (in the issue discussion there are other suggestions that seem promising as well). For the benchmarks please use |
pochmann benchmarked this issue's candidate fixes on pythongh-154594 and found the try/except form is ~3.5x slower on a memo miss (the common case in real deepcopy workloads) despite being fastest on a hit. `d in memo` ties or beats the current `is not None` check on both hit and miss, and matches where the competing PR (pythongh-154595) landed after the same discussion.
|
@eendebakpt Per your request, I benchmarked this with pyperf and pyperf (single-threaded, release build, hit-heavy = repeated shared objects, miss-heavy = all-unique objects):
Takeaways:
I've updated this PR to use |
|
@sreehariannam Thank you for the benchmarks. It seems to |
Fixes gh-154594.
copy.deepcopy()usesmemo.get(d, None)to check the memo dict, treatingNoneboth as the "not cached" sentinel and as a value a__deepcopy__can legitimately return. When an object's__deepcopy__returnsNone, the memo hit is indistinguishable from a miss, so the object gets deep-copied again on every subsequent reference instead of once:This was introduced by gh-132657 / GH-138429, which replaced the previous
_nil = []sentinel withNonespecifically becauseNoneis an immortal singleton in CPython, avoiding refcount contention under free-threading that a plain sentinel object would reintroduce.The fix switches the lookup to
if d in memo: return memo[d](as suggested by @pochmann in the issue discussion), rather than reintroducing a sentinel object or special-casingNone. This distinguishes every possible cached value (includingNone) from a genuine cache miss, without any shared sentinel object and without the exception-handling overhead atry/except KeyErrorform would add on every miss.I benchmarked three candidate approaches (current
is not Nonecheck,try/except KeyError, andd in memo) with pyperf on hit-heavy and miss-heavy workloads, and withTools/ftscalingbenchunder free-threading:is not None)try/exceptd in memo(this PR)d in memoties or beats the current baseline on every axis. The earliertry/exceptversion of this PR was ~17% slower on misses (the common case, per discussion in the issue) and scaled worse under free-threading contention;d in memodoesn't have either regression.Added a regression test and a changelog entry.