cpython
cpython copied to clipboard
python
gh-72793: C implementation of parts of copy.deepcopy
The original idea and implementation are from @villemoes. The original issue has been inactive for a long time.
Benchmark on deepcopy of a dict and dataclass:
deepcopy dict: Mean +- std dev: [b] 7.28 us +- 0.27 us -> [d] 982 ns +- 24 ns: 7.41x faster
deepcopy dataclass: Mean +- std dev: [b] 6.49 us +- 0.07 us -> [d] 3.69 us +- 0.11 us: 1.76x faster
Geometric mean: 3.61x faster
Benchmark details
Test script
import pyperf
runner = pyperf.Runner()
setup="""
import copy
a={'list': [1,2,3,43], 't': (1,2,3), 'str': 'hello', 'subdict': {'a': True}}
from dataclasses import dataclass
@dataclass
class A:
a : list
b : str
c : bool
dc=A([1,2,3], 'hello', True)
"""
runner.timeit(name=f"deepcopy dict", stmt=f"b=copy.deepcopy(a)", setup=setup)
runner.timeit(name=f"deepcopy dataclass", stmt=f"b=copy.deepcopy(dc)", setup=setup)
Fixes #72793
Pyperformance results
Pyperformance results show small speedup, although this could very well be a random fluctuation. (there are no explicit calls to deepcopy in the pyperformance tests)
2to3: Mean +- std dev: [base] 325 ms +- 5 ms -> [patch] 322 ms +- 4 ms: 1.01x faster
chaos: Mean +- std dev: [base] 87.8 ms +- 0.8 ms -> [patch] 88.3 ms +- 1.1 ms: 1.01x slower
float: Mean +- std dev: [base] 103 ms +- 2 ms -> [patch] 98.5 ms +- 2.5 ms: 1.04x faster
go: Mean +- std dev: [base] 163 ms +- 1 ms -> [patch] 165 ms +- 2 ms: 1.01x slower
json_dumps: Mean +- std dev: [base] 15.1 ms +- 0.5 ms -> [patch] 14.9 ms +- 0.1 ms: 1.01x faster
json_loads: Mean +- std dev: [base] 28.0 us +- 0.3 us -> [patch] 28.1 us +- 0.3 us: 1.01x slower
logging_format: Mean +- std dev: [base] 8.11 us +- 0.14 us -> [patch] 8.18 us +- 0.12 us: 1.01x slower
logging_silent: Mean +- std dev: [base] 133 ns +- 1 ns -> [patch] 130 ns +- 1 ns: 1.02x faster
meteor_contest: Mean +- std dev: [base] 128 ms +- 1 ms -> [patch] 126 ms +- 1 ms: 1.01x faster
nbody: Mean +- std dev: [base] 111 ms +- 2 ms -> [patch] 113 ms +- 2 ms: 1.01x slower
nqueens: Mean +- std dev: [base] 106 ms +- 1 ms -> [patch] 106 ms +- 1 ms: 1.00x slower
pathlib: Mean +- std dev: [base] 23.0 ms +- 0.4 ms -> [patch] 23.2 ms +- 0.7 ms: 1.01x slower
pickle: Mean +- std dev: [base] 10.5 us +- 0.1 us -> [patch] 10.6 us +- 0.4 us: 1.01x slower
pickle_dict: Mean +- std dev: [base] 30.9 us +- 0.2 us -> [patch] 31.3 us +- 0.3 us: 1.02x slower
pickle_list: Mean +- std dev: [base] 4.41 us +- 0.04 us -> [patch] 4.55 us +- 0.06 us: 1.03x slower
pyflate: Mean +- std dev: [base] 532 ms +- 8 ms -> [patch] 535 ms +- 4 ms: 1.01x slower
regex_compile: Mean +- std dev: [base] 163 ms +- 1 ms -> [patch] 165 ms +- 1 ms: 1.01x slower
regex_dna: Mean +- std dev: [base] 238 ms +- 2 ms -> [patch] 213 ms +- 1 ms: 1.12x faster
regex_effbot: Mean +- std dev: [base] 3.50 ms +- 0.03 ms -> [patch] 3.08 ms +- 0.03 ms: 1.14x faster
regex_v8: Mean +- std dev: [base] 29.2 ms +- 0.7 ms -> [patch] 25.8 ms +- 0.9 ms: 1.13x faster
richards: Mean +- std dev: [base] 57.4 ms +- 1.5 ms -> [patch] 58.2 ms +- 1.3 ms: 1.01x slower
scimark_fft: Mean +- std dev: [base] 447 ms +- 10 ms -> [patch] 452 ms +- 2 ms: 1.01x slower
scimark_lu: Mean +- std dev: [base] 142 ms +- 2 ms -> [patch] 145 ms +- 1 ms: 1.02x slower
scimark_sor: Mean +- std dev: [base] 155 ms +- 1 ms -> [patch] 156 ms +- 2 ms: 1.01x slower
scimark_sparse_mat_mult: Mean +- std dev: [base] 6.18 ms +- 0.11 ms -> [patch] 6.03 ms +- 0.06 ms: 1.02x faster
spectral_norm: Mean +- std dev: [base] 150 ms +- 7 ms -> [patch] 146 ms +- 3 ms: 1.02x faster
unpack_sequence: Mean +- std dev: [base] 52.6 ns +- 1.0 ns -> [patch] 52.2 ns +- 0.6 ns: 1.01x faster
unpickle_list: Mean +- std dev: [base] 5.77 us +- 0.06 us -> [patch] 5.88 us +- 0.10 us: 1.02x slower
xml_etree_parse: Mean +- std dev: [base] 176 ms +- 3 ms -> [patch] 173 ms +- 2 ms: 1.02x faster
Benchmark hidden because not significant (17): deltablue, fannkuch, hexiom, logging_simple, pickle_pure_python, pidigits, python_startup, python_startup_no_site, raytrace, scimark_monte_carlo, sqlite_synth, telco, unpickle, unpickle_pure_python, xml_etree_iterparse, xml_etree_generate, xml_etree_process
Geometric mean: 1.01x faster
Notes
- Issue: gh-72793
- We keep the original python code for the implementation of
copy.deepcopy(see https://peps.python.org/pep-0399/). In the CI we test both the pure python and accelerator version ofdeepcopy.
@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 C deepcopy method (and vice versa). therefore we cannot test both the fallback and C on the same inputs. The deepcopy is the public method (and _deepcopy_fallback is called via deepcopy), so I think we only need to test on deepcopy. 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 funny try-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 can import copy._deepcopy_fallback and import _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 phrase I 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 the copy 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 can
import copy._deepcopy_fallbackandimport _copy.deepcopyto test the functions independently. If I understand correctly, in your current patch_deepcopy_fallbackis 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_module
copy_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 the copy module there are, and whether there already is a timeline on addressing them?
@serhiy-storchaka Could you indicate which issues with the
copymodule there are, and whether there already is a timeline on addressing them?
See https://github.com/orgs/python/projects/9. Note that some issues are common for the pickle and copy modules. Some issues are about copying concrete classes, they can be ignored in this context. But https://github.com/python/cpython/issues/93627 is specially about inconsistency between implementations, it should be addressed first.
@tiran Thanks for reviewing, with your feedback the PR is in much better shape now. I addressed the comments (except for the one about _Py_hashtable_t which I still have to look into)
Before we continue with reviews and improvements, could you align with the other core dev @serhiy-storchaka who wanted to tackle some other issues with copy/deepcopy before continuing with this PR?
Output of the pyperformance deepcopy benchmark:
deepcopy: Mean +- std dev: [main] 414 us +- 4 us -> [pr] 128 us +- 2 us: 3.23x faster
deepcopy_reduce: Mean +- std dev: [main] 3.69 us +- 0.03 us -> [pr] 2.15 us +- 0.07 us: 1.72x faster
deepcopy_memo: Mean +- std dev: [main] 45.5 us +- 0.6 us -> [pr] 6.71 us +- 0.13 us: 6.77x faster
Geometric mean: 3.35x faster
Thanks for making the requested changes!
@tiran: please review the changes made to this pull request.
Thanks! I'll come back to your PR once 3.11 has reached RC phase.
@tiran Would you have time to review the PR?
@serhiy-storchaka @tiran The behaviour of the new deepcopy implementation is different from the current one for the following case:
import copy
class A:
# class with evil deepcopy
def __init__(self, l):
self.l =l
def __deepcopy__(self, memo):
if len(self.l)>3:
print('deepcopy of A: popping element')
l.pop(-1)
return A(self.l)
l = [1,2,3,4]
l.append(A(l))
l+=[5,6,7]
print(l)
k=copy.deepcopy(l)
print(k)
Output of current main:
[1, 2, 3, 4, <__main__.A object at 0x0000025F66701B40>, 5, 6, 7]
deepcopy of A: popping element
[1, 2, 3, 4, <__main__.A object at 0x0000025F66702B90>, 5, 6]
Output of this PR
[1, 2, 3, 4, <__main__.A object at 0x0000018B6916FFE0>, 5, 6, 7]
deepcopy of A: popping element
[1, 2, 3, 4, <__main__.A object at 0x0000018B6916FFB0>, 5, 6, 7]
There are no tests covering this case that I could find.
It would be good to add a testcase that the new implementation (which is a C extension) does not crash (I can do that for this PR). But should we also specify how deepcopy should behave for cases like this (and add tests for that)?
@tiran Would you be able to continue the review? If not, I will try to find another core dev to review.
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 phrase I 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.
I have made the requested changes; please review again.
If there are any more changes required for PEP7, let me know
Thanks for making the requested changes!
@erlend-aasland, @tiran, @kumaraditya303: please review the changes made to this pull request.
@erlend-aasland Will you be able to continue reviewing the PR?
Yes, sorry for the delay. I can review it by next weekend.
@erlend-aasland Will you be able to continue reviewing the PR?
Yes, sorry for the delay. I can review it by next weekend.
@erlend-aasland Gentle ping
I see that @serhiy-storchaka requested other issues to be resolved before continuing with this PR. I think his comment should be followed up before landing this.
See https://github.com/python/cpython/pull/91610#issuecomment-1150662111
I see that @serhiy-storchaka requested other issues to be resolved before continuing with this PR. I think his comment should be followed up before landing this.
See #91610 (comment)
@erlend-aasland @serhiy-storchaka I created https://github.com/python/cpython/pull/103035 as one of the options to resolve #93627
I see that @serhiy-storchaka requested other issues to be resolved before continuing with this PR. I think his comment should be followed up before landing this. See #91610 (comment)
@erlend-aasland @serhiy-storchaka I created #103035 as one of the options to resolve #93627
Thanks! I'm unable to review that myself, but I've asked for reviewers on the core dev Discord chat. Hopefully someone will pick it up.
FYI, test_copy is failing in the Hypothesis CI: https://github.com/python/cpython/actions/runs/6403563044/job/17382333727?pr=91610