| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Two small comments, but I am leaning more towards the approach suggested in the corresponding issue.
Sorry, something went wrong.
Addresses review: clearer, no sentinel, and the test treats memo as opaque.
|
Switched to if d in memo: return memo[d] as suggested. Drops the sentinel, and the test no longer touches memo. Microbenchmark (5M lookups): d in memo is ~6 ns faster on a hit and ~22 ns on a miss than memo.get(d, None); full deepcopy is within noise. |
Sorry, something went wrong.
Faster in both cases? Did you measure purely d in memo and memo.get(d, None)? Or did you include the other differences as well? |
Sorry, something went wrong.
|
The earlier numbers included the surrounding statements. Measuring just the lookup, each returning the value (best of 5 runs of 10M): hit (present) miss (absent) memo.get(d, None) 51 ns 60 ns memo[d] if d in memo else None 55 ns 49 ns So d in memo is about 4 ns slower on a hit and 11 ns faster on a miss. In deepcopy each object is looked up once, so misses are the common case, and full deepcopy timings are within noise either way. |
Sorry, something went wrong.
|
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 is not None check in an if-context and the store/load of y. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Two small nits, but this looks good to me.
Sorry, something went wrong.
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.
There was a problem hiding this comment.
The code is a bit cleaner, a bit faster and it solves a (minor) bug.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR #138429 (gh-132657) replaced the memo-miss sentinel from _nil = [] to None for free-threading performance. But None is a valid deepcopy result, so memo.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.