Uh oh!
There was an error while loading. Please reload this page.
gh-154594: Fix deepcopy memo lookup when __deepcopy__ returns None - #154595
gh-154594: Fix deepcopy memo lookup when __deepcopy__ returns None#154595tonghuaroot wants to merge 4 commits into
Conversation
eendebakpt
left a comment
There was a problem hiding this comment.
Two small comments, but I am leaning more towards the approach suggested in the corresponding issue.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Addresses review: clearer, no sentinel, and the test treats memo as opaque.
tonghuaroot
commented
Jul 30, 2026
Switched to Microbenchmark (5M lookups): |
pochmann
commented
Jul 30, 2026
Faster in both cases? Did you measure purely |
tonghuaroot
commented
Jul 30, 2026
The earlier numbers included the surrounding statements. Measuring just the lookup, each returning the value (best of 5 runs of 10M): So |
pochmann
commented
Jul 30, 2026
Ok if the surrounding stuff was properly included, then that's good. This new comparison for "just the lookup" is not, it's missing the |
pochmann
commented
Jul 30, 2026
Anyway, I agree that misses are probably the common case, and I'm not surprised if full deepcopy timings are within noise either way, as deepcopy is rather slow. |
eendebakpt
left a comment
There was a problem hiding this comment.
Two small nits, but this looks good to me.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
left a comment
There was a problem hiding this comment.
The code is a bit cleaner, a bit faster and it solves a (minor) bug.
PR #138429 (gh-132657) replaced the memo-miss sentinel from
_nil = []toNonefor free-threading performance. ButNoneis a valid deepcopy result, somemo.get(d, None)can't distinguish "not found" from "found None". This skips memoization for None-valued results, causing redundant__deepcopy__calls.Fix: use a private
_MEMO_MISS = object()sentinel. Immortalized objects have no refcount contention, so the FT benefit is preserved.