-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
subtyping: apply performance optimization in more places #31807
Conversation
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
9bfe621
to
2b64b2b
Compare
…bviously_disjoint
also need to address some bugs in the optimizer since supertype(T=typeof(Union{})) is awkwardly equal to T, even though they are also expected to be distinguishable, and similarly, we might get tripped up to discover that `Tuple{T}` where `isconcretetype(T)` has does subtypes and so these optimizations were being too aggressive also consider the whole Vararg tail, looking for obvious conflicts this skips validation for malformed types with free type vars
t = jl_unwrap_unionall(t); | ||
if (jl_is_datatype(t)) { | ||
if (jl_is_type_type(t)) | ||
return 0; // Type{T} may have the concrete supertype `typeof(T)`, so don't try to handle them here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was the other way around --- that typeof(T)
could have Type{T}
as its supertype?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
julia> T = Int; Type{T} <: typeof(T)
true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I thought you meant the .super
field.
@@ -227,8 +227,8 @@ let ft = Base.datatype_fieldtypes | |||
@test ft(elT2.body)[1].parameters[1] === elT2 | |||
@test Base.isconcretetype(ft(elT2.body)[1]) | |||
end | |||
struct S22624{A,B,C} <: Ref{S22624{Int64,A}}; end | |||
@test @isdefined S22624 | |||
#struct S22624{A,B,C} <: Ref{S22624{Int64,A}}; end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we should try to merge #25796 first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they end up being somewhat unrelated
marked draft, because it'll conflict with #31698, and there will need to be more improvements/corrections to jl_obvious_subtype after that is merged