Make an inference hot-path slightly faster #44421
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
I'm gonna look into the optimizer benchmark to see if there's anything interesting, but it's probably just doing real work to process the new case. That's expected. |
This aims to improve performance of inference slightly by removing a dynamic dispatch from calls to `widenwrappedconditional`, which appears in various hot paths and showed up in profiling of inference. There's two changes here: 1. Improve inlining for calls to functions of the form ``` f(x::Int) = 1 f(@nospecialize(x::Any)) = 2 ``` Previously, we would peel of the `x::Int` case and then generate a dynamic dispatch for the `x::Any` case. After this change, we directly emit an `:invoke` for the `x::Any` case (as well as enabling inlining of it in general). 2. Refactor `widenwrappedconditional` itself to avoid a signature with a union in it, since ironically union splitting cannot currently deal with that (it can only split unions if they're manifest in the call arguments).
|
Yeah, I took a look and as far as I can tell, the optimizer is just doing more real work to provide that inference speedup. Also, my timing didn't show the timing differences as dramatic as what nanosolider shows, just a few percent (but directionally aligned with what nanosolider says). |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
vtjnash
approved these changes
Mar 3, 2022
aviatesk
added a commit
that referenced
this pull request
Jan 30, 2023
#44421 changed the union-splitting to skip generating unnecessary fallback dynamic dispatch call when there is any fully covered call. But it turned out that this is only valid when there is any fully covered call in matches for all signatures that inference split, and it is invalid if there is any union split signature against which any uncovered call is found. Consider the following example: # case 1 # def nosplit(::Any) = [...] nosplit(::Int) = [...] # call nosplit(a::Any) split1: a::Any ┬ nosplit(a::Int) └ nosplit(a::Any) # fully covers split1 # case 2 # def convert(::Type{T}, ::T) = T # call convert(::Type{Union{Bool,Tuple{Int,String}}}, a::Union{Bool,Tuple{Int,Any}}) split1: a::Bool ─ convert(::Type{Bool}, ::Bool) # fully covers split1 split2: a::Tuple{Int,Any} ─ convert(::Type{Tuple{Int,String}}, ::Tuple{Int,String}) # NOT fully covers split2 #44421 allows us to optimize the the first case, but handles the second case wrongly. This commit fixes it up while still optimizing the first case. fix #48397.
aviatesk
added a commit
that referenced
this pull request
Jan 30, 2023
#44421 changed the union-splitting to skip generating unnecessary fallback dynamic dispatch call when there is any fully covered call. But it turned out that this is only valid when there is any fully covered call in matches for all signatures that inference split, and it is invalid if there is any union split signature against which any uncovered call is found. Consider the following example: # case 1 # def nosplit(::Any) = [...] nosplit(::Int) = [...] # call nosplit(a::Any) split1: a::Any ┬ nosplit(a::Int) └ nosplit(a::Any) # fully covers split1 # case 2 # def convert(::Type{T}, ::T) = T # call convert(::Type{Union{Bool,Tuple{Int,String}}}, a::Union{Bool,Tuple{Int,Any}}) split1: a::Bool ─ convert(::Type{Bool}, ::Bool) # fully covers split1 split2: a::Tuple{Int,Any} ─ convert(::Type{Tuple{Int,String}}, ::Tuple{Int,String}) # NOT fully covers split2 #44421 allows us to optimize the the first case, but handles the second case wrongly. This commit fixes it up while still optimizing the first case. fix #48397.
aviatesk
added a commit
that referenced
this pull request
Jan 30, 2023
#44421 changed the union-splitting to skip generating unnecessary fallback dynamic dispatch call when there is any fully covered call. But it turned out that this is only valid when there is any fully covered call in matches for all signatures that inference split, and it is invalid if there is any union split signature against which any uncovered call is found. Consider the following example: # case 1 # def nosplit(::Any) = [...] nosplit(::Int) = [...] # call nosplit(a::Any) split1: a::Any ┬ nosplit(a::Int) └ nosplit(a::Any) # fully covers split1 # case 2 # def convert(::Type{T}, ::T) = T # call convert(::Type{Union{Bool,Tuple{Int,String}}}, a::Union{Bool,Tuple{Int,Any}}) split1: a::Bool ─ convert(::Type{Bool}, ::Bool) # fully covers split1 split2: a::Tuple{Int,Any} ─ convert(::Type{Tuple{Int,String}}, ::Tuple{Int,String}) # NOT fully covers split2 #44421 allows us to optimize the the first case, but handles the second case wrongly. This commit fixes it up while still optimizing the first case. fix #48397.
aviatesk
added a commit
that referenced
this pull request
Jan 30, 2023
#44421 changed the union-splitting to skip generating unnecessary fallback dynamic dispatch call when there is any fully covered call. But it turned out that this is only valid when there is any fully covered call in matches for all signatures that inference split, and it is invalid if there is any union split signature against which any uncovered call is found. Consider the following example: # case 1 # def nosplit(::Any) = [...] nosplit(::Int) = [...] # call nosplit(a::Any) split1: a::Any ┬ nosplit(a::Int) └ nosplit(a::Any) # fully covers split1 # case 2 # def convert(::Type{T}, ::T) = T # call convert(::Type{Union{Bool,Tuple{Int,String}}}, a::Union{Bool,Tuple{Int,Any}}) split1: a::Bool ─ convert(::Type{Bool}, ::Bool) # fully covers split1 split2: a::Tuple{Int,Any} ─ convert(::Type{Tuple{Int,String}}, ::Tuple{Int,String}) # NOT fully covers split2 #44421 allows us to optimize the the first case, but handles the second case wrongly. This commit fixes it up while still optimizing the first case. fix #48397.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This aims to improve performance of inference slightly by removing
a dynamic dispatch from calls to
widenwrappedconditional, whichappears in various hot paths and showed up in profiling of inference.
There's two changes here:
Previously, we would peel of the
x::Intcase and thengenerate a dynamic dispatch for the
x::Anycase. Afterthis change, we directly emit an
:invokefor thex::Anycase (as well as enabling inlining of it in general).
widenwrappedconditionalitself to avoid a signaturewith a union in it, since ironically union splitting cannot currently
deal with that (it can only split unions if they're manifest in the
call arguments).