-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-132657: Make deepcopy and copy scale with free-threading #138429
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
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.
LGTM.
| if memo is None: | ||
| memo = {} | ||
| else: | ||
| y = memo.get(d, _nil) |
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.
I'm surprised, but replacing _nil with None works well. It seems like memo values are never None.
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.
Correct, the only values set are integers (id of objects)
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.
This will break the user code that patches these sets, but this is not a big deal. There is a slower alternative -- modify copyreg.dispatch_table by using the public API copyreg.pickle(). LGTM. 👍
As for _deepcopy_dispatch() -- I am planning to add a public API for modifying it, so it cannot be frozen.
And note that this performance gain is not for long. We will need to get rid of None in favor of other sentinel object. See #109498.
Co-authored-by: Victor Stinner <[email protected]>
|
|
We improve scaling by:
frozensetinstead of asetfor atomic type lookup (this avoids locks)_nilsentinel. The_nilis not immortal, so results in some refcount contention.On the benchmark from #132658 this results in:
Main:
PR:
Remaining work to be done (followup PRs):
copy.deepcopy,_atomic_typesand_deepcopy_dispatchdo not have deferred referenence counting or immortality, so there is some refcount contention left._deepcopy_dispatchis adict. Once there is afrozendictin cpython, we should use that instead.