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

Make deepcopy_internal inferrable for BigInt and BigFloat #54496

Merged

Conversation

lgoettgens
Copy link
Contributor

@lgoettgens lgoettgens commented May 16, 2024

I noticed this is not inferrable/typestable when looking at a deepcopy_internal implementation of one of my own types using JET.

deepcopy_internal(a::ResElem, dict::IdDict) =
   parent(a)(deepcopy_internal(data(a), dict))

data(a) is here a BigInt (and julia can infer this in 1.10), but the type of deepcopy_internal(data(a), dict) gets inferred as Any, thus leading to dynamic dispatch in the surrounding code.

This PR fixes this.

@LilithHafner LilithHafner added the needs tests Unit tests are required for this change label May 17, 2024
@LilithHafner
Copy link
Member

LGTM with some @inferred tests. (probably good to put those tests in test/copy.jl next to @test (@inferred Base.deepcopy_internal(zeros(), IdDict())) == zeros().

@LilithHafner LilithHafner added status:merge me PR is reviewed. Merge when all tests are passing and removed needs tests Unit tests are required for this change labels May 18, 2024
@lgoettgens
Copy link
Contributor Author

Thanks @LilithHafner for adding tests.
Is this suitable for a backport?

@LilithHafner
Copy link
Member

Yes. Bugfixes and simple, unobjectionable performance improvements are backportable. This is the latter.

@LilithHafner LilithHafner merged commit c28a9de into JuliaLang:master May 18, 2024
8 of 9 checks passed
@LilithHafner LilithHafner added backport 1.11 Change should be backported to release-1.11 and removed status:merge me PR is reviewed. Merge when all tests are passing labels May 18, 2024
@lgoettgens lgoettgens deleted the lg/deepcopy-typestable-bigint branch May 18, 2024 19:51
KristofferC pushed a commit that referenced this pull request May 20, 2024
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
(cherry picked from commit c28a9de)
KristofferC pushed a commit that referenced this pull request May 23, 2024
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
(cherry picked from commit c28a9de)
fingolfin pushed a commit that referenced this pull request May 23, 2024
Same as #54496 for some more
methods.
I added some more testcases than the changed methods. For
`SimpleVector`, I don't know how to construct an instance for a
testcase.

This could be backported to 1.11.
KristofferC pushed a commit that referenced this pull request May 27, 2024
Same as #54496 for some more
methods.
I added some more testcases than the changed methods. For
`SimpleVector`, I don't know how to construct an instance for a
testcase.

This could be backported to 1.11.

(cherry picked from commit 7273b9a)
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants