-
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: pgo/devirt diagnostic improvements #53247
Conversation
Several changes to help better diagnose PGO and devirtualization issues: * Report the source of the PGO data to the jit * Report the reason for a devirtualization failure to the jit * Add checking mode that compares result of devirtualzation to class profile * Add reporting mode to assess overall rates of devirtualization failure when the jit has exact type information. Also fix a loophole where in some case we'd still devirtualize if not optimizing. Note crossgen2 does not yet set devirtualization failure reasons.
cc @dotnet/jit-contrib @davidwrighton A lot of files touched as I've modified 2 apis on the jit interface. No diffs (per PMI). |
src/coreclr/inc/corinfo.h
Outdated
// | ||
CORINFO_METHOD_HANDLE devirtualizedMethod; | ||
bool requiresInstMethodTableArg; | ||
CORINFO_CONTEXT_HANDLE exactContext; | ||
const char* failureReason; |
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.
Please describe the memory behavior of this pointer. In particular, is it expected to be remain across multiple calls to resolveVirtulaMethod?
Also, superpmi is not currently storing/restoring this value.
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.
Currently it is only used immediately after the call.
I suppose it would be simpler to make it an enum. I made it a string because I anticipated there would be a lot of churn as we evolve this and didn't want to also have to rev the jit interface each time.
I don't expect actual behavior of the jit to depend on this -- it is just there for diagnostics -- so perhaps an enum with an understanding that a given jit build might see values outside its known enum range will work.
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.
A string is a fine choice. I just want to document its memory behavior, and used directly after the call is a simple and fairly common behavior in the jit interface. The only concern is that handling of strings across the managed/native boundary needs to have some care taken around object lifetime, etc, and writing down the contract is necessary.
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 went ahead and changed it over to an enum, it's simple for SPMI to deal with.
Running Pri1 tests with
gives the following exact class devirt failure breakdown:
Many of the failures in of that first category are cases where we have a shared interface method. |
Several changes to help better diagnose PGO and devirtualization issues:
when the jit has exact type information.
Also fix a loophole where in some case we'd still devirtualize if not
optimizing.
Note crossgen2 does not yet set devirtualization failure reasons.