-
-
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
reduce MAX_TYPEUNION_COMPLEXITY
to 2
#30833
Conversation
Does it make sense to put this into |
41192a0
to
4db64f2
Compare
test/compiler/inference.jl
Outdated
@test Core.Compiler.tmerge(Int32, Union{Int16, Nothing, Tuple{ComplexF32, ComplexF32}}) == | ||
Union{Int16, Int32, Nothing, Tuple{ComplexF32, ComplexF32}} | ||
@test Core.Compiler.tmerge(Union{Int32, Nothing, Tuple{ComplexF32}}, Union{Int16, Nothing, Tuple{ComplexF32, ComplexF32}}) == | ||
@test Core.Compiler.tmerge(Union{Int32, Nothing, Tuple{ComplexF32}}, Union{Int32, Nothing, Tuple{ComplexF32, ComplexF32}}) == | ||
Union{Int16, Int32, Nothing, Tuple{Vararg{ComplexF32}}} |
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.
how does Int16 get in there?
I'm not sure; this mostly controls how fast we widen when type information isn't too good, so I suspect there aren't many use cases. |
4db64f2
to
6c98d1e
Compare
Aha, one use of this limit was comparing with |
@nanosoldier |
Ah, good catch. |
Any numbers to share? Just curious (and presumably some benchmarking justified this). |
It reduces the time to first plot by 1 second. Disappointing, but I think still makes sense since it restores what we did in 0.6. There could also very well be other cases that benefit more. |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
6c98d1e
to
bae54bd
Compare
@nanosoldier |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan |
Ok, I think we actually need two kinds of limits here. One is on "length", i.e. |
This helps make the compiler more efficient. In v0.6 the limit was unions with 3 elements, i.e.
Union{A,B,C}
. However the meaning of the variable was changed to refer to the number of 2-argument union constructors involved, so setting it to 3 allowed unions with 4 elements. This PR reduces the limit down to the same level as in v0.6.