-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
invoke
d calls: record invoke signature in backedges (#46010)
This fixes a long-standing issue with how we've handled `invoke` calls with respect to method invalidation. When we load a package, we need to ask whether a given MethodInstance would be compiled in the same way now (aka, in the user's running session) as when the package was precompiled; in practice, the way we do that is to test whether the dispatches would be to the same methods in the current world-age. `invoke` presents special challenges because it allows the coder to deliberately select a different method than the one that would be chosen by ordinary dispatch; if there is no record of how this choice was made, it can look like it resolves to the wrong method and this can trigger invalidation. This allows a MethodInstance to store dispatch tuples as well as other MethodInstances among their backedges. Additionally: - provide backedge-iterators for both C and Julia that abstracts the specific storage mechanism. - fix a bug in the CodeInstance `relocatability` field, where methods that only return a constant (and hence store `nothing` for `inferred`) were deemed non-relocatable. - fix a bug in which #43990 should have checked that the method had not been deleted. Tests passed formerly simply because we weren't caching external CodeInstances that inferred down to a `Const`; fixing that exposed the bug. This bug has been exposed since merging #43990 for non-`Const` inference, and would affect Revise etc. Co-authored-by: Jameson Nash <[email protected]>
- Loading branch information
Showing
16 changed files
with
585 additions
and
150 deletions.
There are no files selected for viewing
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
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
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
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
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
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
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
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
Oops, something went wrong.
dd375e1
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.
@nanosoldier
runbenchmarks("inference", vs="@3b1c54d91fe8ed9965ba9dc4880530c714c3f82b")
dd375e1
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.
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.
dd375e1
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.
This commit seems to have introduced some regressions in the
inference
benchmarks. The regression inquadratic
example seems to be critical especially.dd375e1
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.
/cc @vtjnash @timholy
dd375e1
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 may have fix this in #46584
dd375e1
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.
It seems like that the very latest
master
is still as slow as this commit.dd375e1
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.
ok, thanks for checking
dd375e1
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.
Profiling indicates (on 31d4c22):
Looking in Cthulhu, there is no reason given that
Core.Compiler.ignorelimited
is very expensive to call and is uninferred (it just returns its argument). It also crashes Cthulhu to check this. I think it is possibly due to our inference-limiting heuristic that we stop inferring methods after the type reachesAny
, unaware that this might thwart significant inlining optimization later.Secondly, this methods needs to add 10k backedges from
+(Int64, Int64) from +(T, T) where {T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8}}
to
quadratic(Int64) from quadratic(Any)
where this methodinstance already has about 5k backedges
and that changed in this PR from being a very cheap pointer comparison for each element in the loop, to requiring it loads the type tag of each element of that array that is very expensive to do
dd375e1
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.
Am I interpreting @vtjnash's analysis as suggesting there is not obviously an easy fix for this issue? We'd have to make the
backedges
list be of homogeneous type but then come up with some other storage mechanism for handlinginvoke
?dd375e1
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 I have fixes on this regression locally. I will try to submit a PR on this.