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

Compiler: avoid type instability in access to PartialStruct field #57553

Merged

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Feb 27, 2025

The Base.getproperty method for PartialStruct has a type assert to ensure type stable access of the field undef. However Compiler.jl has Compiler.getproperty === Core.getfield.

Introduce a getter for this field of PartialStruct into Compiler.jl and use it.

I guess this should improve compiler performance, and it should help avoid spurious invalidation.

@nsajko
Copy link
Contributor Author

nsajko commented Feb 27, 2025

help avoid spurious invalidation

using SnoopCompileCore
invs = @snoop_invalidations begin
    struct I <: Integer end
    Base.Int64(::I) = 7
end
display(length(invs))  # before: 5451, after: 2131

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

Good catch, do you want to merge this before #57541?

The `Base.getproperty` method for `PartialStruct` has a type assert to
ensure type stable access of the field `undef`. However Compiler.jl
has `Compiler.getproperty === Core.getfield`.

Introduce a getter for this field of `PartialStruct` into Compiler.jl
and use it.

I guess this should improve compiler performance, and it should help
avoid spurious invalidation.
@nsajko nsajko force-pushed the Compiler_PartialStruct_undef_field_access branch from 4c45fa3 to 105ffb9 Compare February 27, 2025 17:31
@nsajko
Copy link
Contributor Author

nsajko commented Feb 27, 2025

Good catch

Thanks!

do you want to merge this before #57541?

That would be nice 😃

@nsajko nsajko added the merge me PR is reviewed. Merge when all tests are passing label Feb 27, 2025
@giordano giordano merged commit f5ce249 into JuliaLang:master Feb 28, 2025
8 checks passed
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants