I think this PR should include a news entry for the performance improvement.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

A few unresolved things:

• dict_factory=dict optimisation will now be done in a separate follow-up PR.
• If I make a test for _ATOMIC_TYPES is there a specific spot it whould go?
• In the discussion thread Eric posted about gettingcopy to handle/provide the list/set of types
  • I think that's out of scope for this change and concealswhy these types matter, but maybe worth looking into if it would be useful elsewhere
  • This could always be changed to use such a set if it was provided and if that was the right choice

With regard to the 'simpler' version, while it is a cleaner change I'm not sure it's the right decision to give up on the performance improvement that we can getnow for the potential that changes to the interpreter might get some of it back in the future?

However I recognise I'm also not the one who has to maintain this and I do understand that it is a bit messier so I'll make the change if that's still requested. (The thumbs up on my comment explaining why I hadn't just done that has left me a bit confused on that front).

If I make a test for _ATOMIC_TYPES is there a specific spot it whould go?

I'm sort-of -0 on importing anything private fromcopy.py, whether it's fordataclasses itself or adataclasses test. I don't like the idea of coupling the logic of the two modules like that.But I'm not adataclasses maintainer, so you should probably listen to Carl and Eric over me on this.

Anyway, the test, if you do add it, should go somewhere inLib/test/test_dataclasses.py.

With regard to the 'simpler' version, while it is a cleaner change I'm not sure it's the right decision to give up on the performance improvement that we can getnow for the potential that changes to the interpreter might get some of it back in the future?

However I recognise I'm also not the one who has to maintain this and I do understand that it is a bit messier so I'll make the change if that's still requested. (The thumbs up on my comment explaining why I hadn't just done that has left me a bit confused on that front).

Another way of looking at that is: should we make the code significantly less readablenow for an optimisation based on an implementation detail of the current interpreter, that could change at any time? Once the code is checked in, it will be hard to justify changing it in the future for readability reasons, as we highly value code stability in the CPython repo -- we generally only make changes if they fix user-visible bugs or are a meaningful performance improvement. The readability loss could be permanent.

I would still favour the simpler code. I'll let@carljm explain the meaning of his thumbs-up ;)

(The thumbs up on my comment explaining why I hadn't just done that has left me a bit confused on that front).

Sorry, my mistake! I've removed the thumbs-up to clarify :) Somehow the first time around I missed the second sentence of that comment, and only saw the first part saying that you had already done the benchmark. I have no explanation for how I managed to miss the second sentence, considering I was actually a bit confused why you were mentioning that you'd already done the benchmark without offering any further conclusions!

Another way of looking at that is: should we make the code significantly less readable now for an optimisation based on an implementation detail of the current interpreter, that could change at any time? Once the code is checked in, it will be hard to justify changing it in the future for readability reasons, as we highly value code stability in the CPython repo -- we generally only make changes if they fix user-visible bugs or are a meaningful performance improvement. The readability loss could be permanent.

I would still favour the simpler code.

I agree with this. There is always a balance between readability and performance, and we don't always prefer any detectable improvement in performance at any cost in readability and maintainability. We can get most of the wins here with much nicer code.

I'm sort-of -0 on importing anything private from copy.py, whether it's for dataclasses itself or a dataclasses test. I don't like the idea of coupling the logic of the two modules like that.

Logically, the coupling is introduced by the optimization itself; the test is just verifying the assumptions of the optimization aren't broken. That said, I think it's very unlikely thatdeepcopy would ever reduce the set of objects that it treats as atomic, and it's more likely they might rearrange the details of how that dispatch happens, so probably the test would be much more likely to fail for spurious and annoying reasons than for real ones. So I've changed my mind: forget the test :)

I think that's all the comments covered?

I'm not sure I'll do a PR for the dict_factory special case, I think that was less significant than skipping the function call overhead? Logically now you'd probably write it as a comprehension (with the inline checks this was much more awkward) and I don't remember that getting as much improvement (perhaps when PEP709 lands).

I have made the requested changes; please review again

Thanks for making the requested changes!

@carljm: please review the changes made to this pull request.

I'm basically okay with this, although as I mentioned on discuss.python.org I'm concerned about maintaining the list of types here and not incopy. This seems to be a generally useful optimization, why enable it only here? Should every module that wants to do something similar maintain it's own copy of_ATOMIC_TYPES?

The way I look at it now is that it's notreally aboutdeepcopy.deepcopy ignoring these objects permits us to special case and return them, but if they weren'tuseful types it would be irrelevant.

The overlap with the types the stdlibjson can handle and the other potentially common types (complex, bytes) are what made this seem worthwhile to me. The remaining types are mostly there because I didn't have a compelling reasonnot to include them? Including everything at least fit the pattern of 'ignored by deepcopy' rather than being an arbitrary subset that I had chosen (and I definitely didn't thinkI should be the one choosing).

I'm concerned about maintaining the list of types here and not incopy. This seems to be a generally useful optimization, why enable it only here? Should every module that wants to do something similar maintain it's own copy of_ATOMIC_TYPES?

I think this is mostly a dataclasses-specific optimization, where one component of it is avoiding an unnecessary call to deepcopy. A chunk of the win comes from short-circuiting the other type checks done by_as*_inner before it even tries deepcopy. Dataclasses, like deepcopy, is implementing a recursive algorithm that should descend only into "non-atomic" types, which is why this optimization can do "double duty" for dataclasses. I doubt it would make sense for other callers of deepcopy who aren't in a similar situation to bother with a pre-check for atomic types.

Given that I don't see other similar uses of deepcopy in the stdlib that would benefit from this, I'm not sure it's worth exposing a new public attribute for this on the copy module? Duplication seems OK here. But I'm also not opposed to making the change incopy to expose it.

@ericvsmith considering my comment above, do you still feel that this should be updated to expose a list of atomic types as an attribute of thecopy module?

@carljm : I'm okay with just leaving it in dataclasses, at least for starters. We can always move it in the future if needed. Feel free to merge this if you have the time and inclination.

The docs failures are a known issue, and are unrelated to this PR.

Thanks@DavidCEllis, this is a great speedup!