| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@pablogsal Would you be able to review this PR? |
Sorry, something went wrong.
|
@serhiy-storchaka As the latest core develop working on this file, would you be able to review this PR? |
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
@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. |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
There are some issues with the copy module. I prefer to resolve them prior to adding the C implementation. |
Sorry, something went wrong.
@tiran noted the fallback will be used by PyPy, so it must indeed be tested. I will add the required tests |
Sorry, something went wrong.
|
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
|
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
@erlend-aasland The failing tests where due to the recent addition of copy.replace. I added the unit tests for new functionality in the same structure as the other unit tests. This means that the pure python method copy.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. |
Sorry, something went wrong.
I'll leave that to @serhiy-storchaka. |
Sorry, something went wrong.
Christian is inactive at the moment, so dismissing his review.
|
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. |
Sorry, something went wrong.
|
@eendebakpt, also consider adapting to free-threading in a follow-up PR; this PR is already a huge diff. |
Sorry, something went wrong.
|
The following commit authors need to sign the Contributor License Agreement: |
Sorry, something went wrong.
|
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 faster deepcopy dataclass: Mean +- std dev: [main] 4.21 us +- 0.08 us -> [pr] 2.57 us +- 0.11 us: 1.64x faster deepcopy small dataclass: Mean +- std dev: [main] 3.03 us +- 0.12 us -> [pr] 2.28 us +- 0.10 us: 1.33x faster deepcopy small tuple: Mean +- std dev: [main] 732 ns +- 21 ns -> [pr] 72.8 ns +- 2.5 ns: 10.05x faster deepcopy repeating: Mean +- std dev: [main] 20.7 us +- 0.7 us -> [pr] 6.49 us +- 0.32 us: 3.18x faster deepcopy repeating_atomic: Mean +- std dev: [main] 10.2 us +- 0.3 us -> [pr] 801 ns +- 33 ns: 12.79x faster Geometric mean: 4.23x faster |
Sorry, something went wrong.
|
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:
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). |
Sorry, something went wrong.
|
If you're looking for a faster deepcopy today, you may find copium useful. It's a complete rewrite of deepcopy in C, compatible with Python 3.10-3.14(t), available on PyPI. It was initially based on this PR, but quickly grew into a much larger project. It's still WIP, but already has extensive test coverage and provides real speed boost. Benchmarks
cat > /tmp/benchmark.py << 'PY'
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)
@dataclass
class A:
a : int
dc_small = A(123)
small_tuple = (1, )
l = {'hi': 100}
repeating_atomic = [ [1] * 100]
repeating = [dc_small] * 100
"""
runner.timeit(name="deepcopy dict", stmt=f"b=copy.deepcopy(a)", setup=setup)
runner.timeit(name="deepcopy dataclass", stmt=f"b=copy.deepcopy(dc)", setup=setup)
runner.timeit(name="deepcopy small dataclass", stmt=f"b=copy.deepcopy(dc_small)", setup=setup)
runner.timeit(name="deepcopy small tuple", stmt=f"b=copy.deepcopy(small_tuple)", setup=setup)
runner.timeit(name="deepcopy repeating", stmt=f"b=copy.deepcopy(repeating)", setup=setup)
runner.timeit(name="deepcopy repeating_atomic", stmt=f"b=copy.deepcopy(repeating_atomic)", setup=setup)
PY
py=3.14
rm -f /tmp/copium_$py.json \
&& uv run --python $py --with 'copium[autopatch]' --with pyperf python /tmp/benchmark.py -o /tmp/copium_$py.json \
&& rm -f /tmp/copy_$py.json \
&& uv run --python $py --with pyperf python /tmp/benchmark.py -o /tmp/copy_$py.json \
&& uvx pyperf compare_to /tmp/copy_$py.json /tmp/copium_$py.json --table --table-format md |
Sorry, something went wrong.
|
@Bobronium Thanks for the link! I tried out copium, and it works as advertised! Historically there have been two reasons for me to pursue this PR:
The second point is handled by copium by patching the vectorcall function of the deepcopy object (see remark Zero overhead patch). Not all projects will be comfortable with such patching, but it works! Unless there is interest from a core dev to review the PR (in which case I will rebase) I am going to close the PR. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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:
Updated benchmark: (23-12-2022, main at 1ecfd1e)
Updated benchmark: (16-9-2023, main at e57ecf6)
Updated benchmark (10-01-2024)
Benchmark details
Test scriptUpdated 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) @dataclass class A: a : int dc_small = A(123) small_tuple = (1, ) l = {'hi': 100} repeating_atomic = [ [1] * 100] repeating = [dc_small] * 100 """ runner.timeit(name="deepcopy dict", stmt=f"b=copy.deepcopy(a)", setup=setup) runner.timeit(name="deepcopy dataclass", stmt=f"b=copy.deepcopy(dc)", setup=setup) runner.timeit(name="deepcopy small dataclass", stmt=f"b=copy.deepcopy(dc_small)", setup=setup) runner.timeit(name="deepcopy small tuple", stmt=f"b=copy.deepcopy(small_tuple)", setup=setup) runner.timeit(name="deepcopy repeating", stmt=f"b=copy.deepcopy(repeating)", setup=setup) runner.timeit(name="deepcopy repeating_atomic", stmt=f"b=copy.deepcopy(repeating_atomic)", setup=setup)Old 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 resultsPyperformance results show small speedup, although this could very well be a random fluctuation. (there are no explicit calls to deepcopy in the pyperformance tests)
Notes