Skip to content
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

gh-100817: Speed up copy.deepcopy calls on slice objects #100818

Closed
wants to merge 1 commit into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jan 7, 2023

See the original issue for microbenchmarks.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR results in a behaviour change for the following edge case.

On main:

>>> s = slice([1, 2], [3, 4], [5, 6])
>>> from copy import deepcopy
>>> t = deepcopy(s)
>>> t.start is s.start
False

With this PR:

>>> s = slice([1, 2], [3, 4], [5, 6])
>>> from copy import deepcopy
>>> t = deepcopy(s)
>>> t.start is s.start
True

@sobolevn
Copy link
Member Author

sobolevn commented Jan 7, 2023

Indeed, I haven't thought about this case. One of my main assumptions was that slice cannot contain nested structures. Because I've only seen int samples in real life. But, if lists are valid and used - this should not be merged. Because deepcopy does not work as promised: it is a shallow copy now.

@sobolevn sobolevn closed this Jan 7, 2023
@AlexWaygood
Copy link
Member

Yeah, it's a really obscure edge case, but I can find some examples in the wild: https://grep.app/search?q=%20slice%28%5B&case=true&words=true&filter[lang][0]=Python.

Would you maybe like to add a test for this edge case? I very nearly merged the PR on the basis that all the tests passed, and it seemed like an impressive optimisation :)

miss-islington pushed a commit that referenced this pull request Jan 11, 2023
CC @AlexWaygood as the reviewer of #100818

Automerge-Triggered-By: GH:AlexWaygood
sobolevn added a commit to sobolevn/cpython that referenced this pull request Jan 12, 2023
CC @AlexWaygood as the reviewer of python#100818

Automerge-Triggered-By: GH:AlexWaygood.
(cherry picked from commit 729ab9b)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
sobolevn added a commit to sobolevn/cpython that referenced this pull request Jan 12, 2023
CC @AlexWaygood as the reviewer of python#100818

Automerge-Triggered-By: GH:AlexWaygood.
(cherry picked from commit 729ab9b)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants