Skip to content

Commit

Permalink
when widening tuple types in tmerge, only widen the complex parts (#5…
Browse files Browse the repository at this point in the history
…0929)

This is the part of #50927
required to fix #49249.
Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)`
would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION,
Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels
bad intuitively because giving the compiler more type information led it
to forget type information that it already knew about, and is especially
damaging because it led to unnecessary type instability when iterating
tuples with complex element types (because the iterator state should be
inferrable as an `Int` even if you have no idea what the tuple type is).

This is tagged for backport to 1.10 since it is a relatively unobtrusive
change and it fixes the string regression in a more proper way.
  • Loading branch information
oscardssmith authored Aug 17, 2023
1 parent 30a73de commit 6e2e6d0
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 12 deletions.
25 changes: 13 additions & 12 deletions base/compiler/typelimits.jl
Original file line number Diff line number Diff line change
Expand Up @@ -764,23 +764,24 @@ end
return u
end
# don't let the slow widening of Tuple cause the whole type to grow too fast
# Specifically widen Tuple{..., Union{lots of stuff}...} to Tuple{..., Any, ...}
for i in 1:length(types)
if typenames[i] === Tuple.name
widen = unwrap_unionall(types[i])
if isa(widen, DataType) && !isvatuple(widen)
widen = NTuple{length(widen.parameters), Any}
else
widen = Tuple
end
types[i] = widen
u = Union{types...}
if issimpleenoughtype(u)
return u
ti = types[i]
tip = (unwrap_unionall(types[i])::DataType).parameters
lt = length(tip)
p = Vector{Any}(undef, lt)
for j = 1:lt
ui = tip[j]
p[j] = (unioncomplexity(ui)==0) ? ui : isvarargtype(ui) ? Vararg : Any
end
break
types[i] = rewrap_unionall(Tuple{p...}, ti)
end
end
# finally, just return the widest possible type
u = Union{types...}
if issimpleenoughtype(u)
return u
end
return Any
end

Expand Down
15 changes: 15 additions & 0 deletions test/compiler/inference.jl
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,21 @@ tmerge_test(Tuple{}, Tuple{Complex, Vararg{Union{ComplexF32, ComplexF64}}},
@test Core.Compiler.tmerge(Base.BitIntegerType, Union{}) === Base.BitIntegerType
@test Core.Compiler.tmerge(Union{}, Base.BitIntegerType) === Base.BitIntegerType
@test Core.Compiler.tmerge(Core.Compiler.fallback_ipo_lattice, Core.Compiler.InterConditional(1, Int, Union{}), Core.Compiler.InterConditional(2, String, Union{})) === Core.Compiler.Const(true)
# test issue behind https://github.com/JuliaLang/julia/issues/50458
@test Core.Compiler.tmerge(Nothing, Tuple{Base.BitInteger, Int}) == Union{Nothing, Tuple{Any, Int}}
@test Core.Compiler.tmerge(Nothing, Tuple{Union{Char, String, SubString{String}, Symbol}, Int}) == Union{Nothing, Tuple{Any, Int}}
@test Core.Compiler.tmerge(Nothing, Tuple{Integer, Int}) == Union{Nothing, Tuple{Integer, Int}}

# test that recursively more complicated types don't widen all the way to Any when there is a useful valid type upper bound
# Specificially test with base types of a trivial type, a simple union, a complicated union, and a tuple.
for T in (Nothing, Base.BitInteger, Union{Int, Int32, Int16, Int8}, Tuple{Int, Int})
Ta, Tb = T, T
for i in 1:10
Ta = Union{Tuple{Ta}, Nothing}
Tb = Core.Compiler.tmerge(Tuple{Tb}, Nothing)
@test Ta <: Tb <: Union{Nothing, Tuple}
end
end

struct SomethingBits
x::Base.BitIntegerType
Expand Down

3 comments on commit 6e2e6d0

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

looks fixed

["problem", "json", "parse_json"]	0.45 (5%) ✅	0.36 (1%) ✅
["problem", "spellcheck", "spellcheck"]	0.23 (5%) ✅	0.31 (1%) ✅

Please sign in to comment.