-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
PGO: GetLikelyClass changes missing logic to enable guarded devirtualization #51643
Comments
There may be other things here to sort out too. If I dump the pgo data from a Release SPC, I see:
Which should say that The likelihood problem comes from this bit of code *pLikelihood = (UINT32)(schema[i].Other && 0xFF); where we should use and so we don't do guarded devirt:
If we run forward to the second call site at offset 233 the returned class handle is Simple-ish repro (using release runtime, checked jit) using System;
using System.Collections.Generic;
using System.Threading;
class X
{
public static void Main()
{
Dictionary<string, string> s = new Dictionary<string, string>();
s["hello"] = "world";
for (int i = 0; i < 100; i++)
{
_ = s["hello"];
Thread.Sleep(15);
}
Thread.Sleep(100);
for (int i = 0; i < 100_000; i++)
{
_ = s["hello"];
}
}
} |
Partial fixes on my branch here: main...AndyAyersMS:UpdateJitForNewLikelyClassRecords |
Sigh. I'll pull that down and start taking a look. |
@AndyAyersMS I've found the fix for that problem, but there is an additional issue I hit with an overeager assert that I'd like you to deal with. See AndyAyersMS#1 for details. |
The jit also can't recreate the exact edge instrumentation schema... not clear what this means. Am going to see if the resolved flow seems plausible. I recall thinking we could suppress schema elements at times, but I don't recall implementing that.
The class profile info seems to line up ok, that is, we have class profiles at offsets 0x34 and xe9 and there are callvirts at those offsets (some of the other callvirts do not have class profiles, but it looks like they're in blocks that were never hit). |
Thanks David. I'll take a look at that assert. |
Looking better now:
|
Fixed via #51664 |
The jit won't attempt guarded devirtualization unless it sees class profile data in the PGO. That analysis needs to be updated to recognize the
GetLikelyClass
record types.So currently the jit is ignoring all this class info. That may explain some of the regressions we saw in microbenchmarks.
Follow-on work from #51284.
The text was updated successfully, but these errors were encountered: