-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
JIT: profile synthesis consistency checking and more #83068
Conversation
Add the ability to verify that the profile counts produced by synthesis are self-consistent, and optionally to assert if they're not. Disable checking if we know profile flow will be inconsistent (because of irreducible loops and/or capped cyclic probabilities). Consistently ignore the likely flow out of handlers. Generally we model handlers as isolated subgraphs that do not contribute flow to the main flow graph. This is generally acceptable. The one caveat is for flow into finallies. The plan here is to fix the weights for finallies up in a subsequent pass via simple scaling once callfinallies are introduced. Flow weights out of finallies will be ignored as the callfinally block will be modeled as always flowing to its pair tail. Also add the ability to propagate the synthesized counts into the live profile data. This is mainly to facilitate using the MIBC comparison tools we have to assess how closely the synthesiszed data comes to actual PGO data. Finally, enable the new synthesized plus checked modes in a few of our PGO pipelines. Contributes to dotnet#82964.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsAdd the ability to verify that the profile counts produced by synthesis are self-consistent, and optionally to assert if they're not. Disable checking if we know profile flow will be inconsistent (because of irreducible loops and/or capped cyclic probabilities). Consistently ignore the likely flow out of handlers. Generally we model handlers as isolated subgraphs that do not contribute flow to the main flow graph. This is generally acceptable. The one caveat is for flow into finallies. The plan here is to fix the weights for finallies up in a subsequent pass via simple scaling once callfinallies are introduced. Flow weights out of finallies will be ignored as the callfinally block will be modeled as always flowing to its pair tail. Also add the ability to propagate the synthesized counts into the live profile data. This is mainly to facilitate using the MIBC comparison tools we have to assess how closely the synthesiszed data comes to actual PGO data. Finally, enable the new synthesized plus checked modes in a few of our PGO pipelines. Contributes to #82964.
|
@BruceForstall PTAL SPMI passes with synthesized counts and checking enabled. So basic algorithm is looking pretty solid (at least running where it does, super early). As for how accurate it is given the very simple heuristics, here is one take: comparing a synthesized MIBC with a measured one using interlocked PGO on TE MvcPlaintext:
This looks fairly good, but may be misleading as there are many simple methods. |
Irreducible loops seem to be somewhat common in async methods. Around 700 cases in ASP.NET collection. Probability caps are rare in actual (non-test code) -- there are only a handful (less than 10) in the ASP.NET collection. |
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.
LGTM
|
||
inline constexpr ProfileChecks operator ~(ProfileChecks a) | ||
{ | ||
return (ProfileChecks)(~(unsigned int)a); |
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 wonder if we should have a CHECK_ALL
that contains all the legal bits (currently 0xF) and have this be:
return (ProfileChecks)(~(unsigned int)a); | |
return (ProfileChecks)(CHECK_ALL & ~(unsigned int)a); |
to prevent returning bits that are not "legal" ProfileChecks
bits.
Could validate/cap the value of JitConfig.JitProfileChecks()
this way, too.
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 is all boilerplate for our common enum class
approach, so we'd probably want to try and handle them all this way.
Since enum class
is (somewhat) type safe seems like we could just mask when converting from int
or other similar types?
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.
Sure, I hadn't noticed that other similar cases had also defined operator~()
without this. A minor thing, to be sure.
x86 failure looks like an AV in the xunit test host runtime? Exit code is
Other failures are known/timeouts. |
Add the ability to verify that the profile counts produced by synthesis are self-consistent, and optionally to assert if they're not. Disable checking if we know profile flow will be inconsistent (because of irreducible loops and/or capped cyclic probabilities).
Consistently ignore the likely flow out of handlers. Generally we model handlers as isolated subgraphs that do not contribute flow to the main flow graph. This is generally acceptable.
The one caveat is for flow into finallies. The plan here is to fix the weights for finallies up in a subsequent pass via simple scaling once callfinallies are introduced. Flow weights out of finallies will be ignored as the callfinally block will be modeled as always flowing to its pair tail.
Also add the ability to propagate the synthesized counts into the live profile data. This is mainly to facilitate using the MIBC comparison tools we have to assess how closely the synthesiszed data comes to actual PGO data.
Finally, enable the new synthesized plus checked modes in a few of our PGO pipelines.
Contributes to #82964.