@pablogsal Would you be able to review this PR?

@serhiy-storchaka As the latest core develop working on this file, would you be able to review this PR?

I don't feel comfortable reviewing the C code, but you will need to add tests to ensure the fallback and C implementation have the same inputs/outputs/etc -- the easiest way would be to duplicate the tests and run one set for the C accelerator and one for the Python version.

A

@AA-Turner The_deepcopy_fallback is only used for objects not handled by the Cdeepcopy method (and vice versa). therefore we cannot test both the fallback and C on the same inputs. Thedeepcopy is the public method (and_deepcopy_fallback is called viadeepcopy), so I think we only need to test ondeepcopy. If there are any tests you would like me to add, let me know.

Note: in earlier versions of the PR the fallback there was a funnytry-except statement to fix a build problem. This I resolved by making_copy a builtin module.

The _deepcopy_fallback is only used for objects not handled by the C deepcopy method (and vice versa). therefore we cannot test both the fallback and C on the same inputs.

If both functions exist and can be theoretically used, both must be tested. You canimport copy._deepcopy_fallback andimport _copy.deepcopy to test the functions independently. If I understand correctly, in your current patch_deepcopy_fallback is only ever called from the C layer. If so, you should make this more clear -- oftentimes the C accelerator has a pure Python fallback which implements the same method when the extension module can't be loaded.

A

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.

There are some issues with thecopy module. I prefer to resolve them prior to adding the C implementation.

The _deepcopy_fallback is only used for objects not handled by the C deepcopy method (and vice versa). therefore we cannot test both the fallback and C on the same inputs.

If both functions exist and can be theoretically used, both must be tested. You canimport copy._deepcopy_fallback andimport _copy.deepcopy to test the functions independently. If I understand correctly, in your current patch_deepcopy_fallback is only ever called from the C layer. If so, you should make this more clear -- oftentimes the C accelerator has a pure Python fallback which implements the same method when the extension module can't be loaded.

A

@tiran noted the fallback will be used by PyPy, so it must indeed be tested. I will add the required tests

We have helper code to block / force imports to test both pure and C accelerated features.

from test.support.import_helper import import_fresh_modulecopy_py = import_fresh_module('copy', blocked=['_copy'])try:    copy_c = import_fresh_module('copy', fresh=['_copy'])except ImportError:    copy_c = None

I addressed some of the review comments.

@serhiy-storchaka Could you indicate which issues with thecopy module there are, and whether there already is a timeline on addressing them?

FYI,test_copy is failing in the Hypothesis CI:https://github.com/python/cpython/actions/runs/6403563044/job/17382333727?pr=91610

FYI,test_copy is failing in the Hypothesis CI:https://github.com/python/cpython/actions/runs/6403563044/job/17382333727?pr=91610

Will look at later this week

@erlend-aasland The failing tests where due to the recent addition ofcopy.replace. I added the unit tests for new functionality in the same structure as the other unit tests. This means that the pure python methodcopy.replace is tested twice (which is a bit redundant, but with better consistency overall).

The PR is ready for review, although I am not sure the item Serhiy raised should be resolved first. See#103035 (comment) and#109498.

The PR is ready for review, although I am not sure the item Serhiy raised should be resolved first. See#103035 (comment) and#109498.

I'll leave that to@serhiy-storchaka.

For any reviewers: I also created a performance improvement for the python implementation:#114266.

Depending on the outcome of that PR and some (minor) changes I want to do to make this work in free-threading I will update the benchmarks.

@eendebakpt, also consider adapting to free-threading in a follow-up PR; this PR is already a huge diff.

The following commit authors need to sign the Contributor License Agreement:

• unknow@example.com

Click the button to sign:

Update of the benchmarks with recent changes to main:

deepcopy dict: Mean +- std dev: [main] 3.75 us +- 0.17 us -> [pr] 587 ns +- 25 ns: 6.40x fasterdeepcopy dataclass: Mean +- std dev: [main] 4.21 us +- 0.08 us -> [pr] 2.57 us +- 0.11 us: 1.64x fasterdeepcopy small dataclass: Mean +- std dev: [main] 3.03 us +- 0.12 us -> [pr] 2.28 us +- 0.10 us: 1.33x fasterdeepcopy small tuple: Mean +- std dev: [main] 732 ns +- 21 ns -> [pr] 72.8 ns +- 2.5 ns: 10.05x fasterdeepcopy repeating: Mean +- std dev: [main] 20.7 us +- 0.7 us -> [pr] 6.49 us +- 0.32 us: 3.18x fasterdeepcopy repeating_atomic: Mean +- std dev: [main] 10.2 us +- 0.3 us -> [pr] 801 ns +- 33 ns: 12.79x fasterGeometric mean: 4.23x faster

To make a fair comparison between current main and the C implementation in this PR I looked at the python implementation again to see whether there are optimizations we do in the C implementation that could also be applied in the Python main. There are quite some options:

• Remove the redundant_nil argument todeepcopy
• Replace try-except constructions with dictionaries with a d.get(key, sentinel) to avoid expensive creation of exceptions
• Use comprehensions in thedeepcopy_list anddeepcopy_tuple
• Indeepcopy_tuple combine the creation ofy and the checkfor k, j in zip(x, y): into a single for loop
• Inline theif cls in _atomic_types: return x part at various locations
• Inline the call to the (only once used)_keep_alive

I will not make PRs for these, because each of them has a very high likelihood of being rejected because either the performance gain is small or the change would reduce readability or maintainability of the code (for the interested reader: I would give the last option the highest probability of success).