-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: Return multiple likely classes in getLikelyClass (for better GDV) #58984
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThis PR extends using System;
using System.Runtime.CompilerServices;
using System.Threading;
class ClassA
{
public virtual int DoWork() => 42;
}
class ClassB : ClassA { }
class ClassC : ClassB { }
public class Program
{
public static void Main()
{
for (int i = 0; i < 1000; i++)
{
if (i % 5 == 0)
Test(new ClassC());
else if (i % 3 == 0)
Test(new ClassB());
else
Test(new ClassA());
Thread.Sleep(5);
}
Console.ReadKey();
}
[MethodImpl(MethodImplOptions.NoInlining)]
static int Test(ClassA? a) => a?.DoWork() ?? 0;
} When it tries to devirtualize
I'm locally playing with it to do multiple type checks for GDV like: if (obj is A)
tmp = (A)a.DoWork();
else if (obj is B)
tmp = (B)b.DoWork();
else if (obj is C)
tmp = (C)c.DoWork();
else
obj.DoWork(); // non-devirtualized fallback or merge multiple checks if they point to the same method (it means if (obj is A || obj is B)
tmp = (A)a.DoWork();
else
obj.DoWork(); // non-devirtualized fallback Btw, here is some statistics I've harvested on AvaloniaILSpy application in FullPGO mode: so non-monomorphic callsites aren't that rare as I thought. /cc @dotnet/jit-contrib @AndyAyersMS @jakobbotsch
|
hm.. where should I rename getLikelyClass for |
It will be interesting to see how this plays out. Couple of notes:
|
Thanks @AndyAyersMS these are useful notes!
I'm going to change
So for now this PR doesn't change anything other than prints more info in JitDump. I'd like to start from the case where the second most popular type uses the same method as the first one. From what I see there are ~2000 such virtual calls in AvaloniaUI app. I hope it won't cause any problem for things like "chained GDV" since I want it to emit something like: single pMT check:
two pMT checks:
so it won't produce any new basic block and should be still thread-jump friendly. I hope it won't affect binary-size/memory footprint much or we can always make inliner a bit less aggressive if overall performance will be better 🙂 |
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
@jakobbotsch btw, any idea what |
I think it's just the internal name used by crossgen2 for the JIT native library. It redirects to the proper dll here: runtime/src/coreclr/tools/Common/JitInterface/JitConfigProvider.cs Lines 44 to 63 in f29484a
So since you already renamed the export and pinvoke name I think it's fine. |
…into gdv-improvement
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
For some reason CI still screams it can't find that entrypoint in clrjitilc while locally it builds just fine :| |
That's weird, I wonder if it's somehow using an outdated JIT dll (maybe from the sdk)? It looks like it's only happening on non-Windows builds. @davidwrighton do you have an idea what could be going on here? |
@EgorBo can you describe the format change to the PgoSchema data. In particular, as this is part of the static pgo format, we need to be certain that the new parser can read the old data. Also, I think I found the spot where you failed to mark the symbol as a public symbol. See my comment above. |
Co-authored-by: Bruce Forstall <brucefo@microsoft.com>
From my understanding, it doesn't change the format anyhow. |
Co-authored-by: Andy Ayers <andya@microsoft.com>
@AndyAyersMS Thanks, addressed. |
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.
Looks good overall. Just a few nits.
I don't suppose we can look at diffs easily. Make sure you spot-check a few cases by hand I guess.
I'm confused why you are seeing null handles. The |
Thanks! So if a histogram contains a few (or even a single) |
The runtime helpers for class profiles perform what's known as reservoir sampling. The data we track includes a count of the number of times the call site was hit. The histogram table can have 0..8 non-null entries with current table size. They can only all be null if the counter is zero, meaning this call site was never hit at runtime (this is fairly common). There may be "trailing" nulls if the call site is lightly hit (count 1...7). If the call site is more frequently hit all entries should be non-null. The same handle may appear in more than one slot in the table, which is how we deduce its relative likelihood. |
96646d0
to
32871ab
Compare
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'm still unsure on one point.
If the observations include collectible classes we put in placeholders (potentially up to 32 different values, though we currently just always use one). And we want the presence of those placeholders to show up in the estimated number of classes seen.
However, we filter the placeholders from the output array, so there's no way the count of elements in the output array can represent the estimated number of classes.
Am I missing something here...?
@AndyAyersMS Sorry for the delay, so currently for for this |
Failures aren't related (fail on other PRs too) |
This PR extends
getLikelyClass
to return multiple likely classes like for the following sample:When it tries to devirtualize
a.DoWork()
it now prints:I'm locally playing with it to do multiple type checks for GDV like:
or merge multiple checks if they point to the same method (it means
DoWork
is not overridden inB
)Btw, here is some statistics I've harvested on AvaloniaILSpy application in FullPGO mode:
(So only 57% of virtual calls with class-probes data are 100% monomorphic in this app)
/cc @dotnet/jit-contrib @AndyAyersMS @jakobbotsch