Skip to content
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

Unclear logic in Compiler::impDevirtualizeCall #43607

Closed
Rattenkrieg opened this issue Oct 19, 2020 · 10 comments
Closed

Unclear logic in Compiler::impDevirtualizeCall #43607

Rattenkrieg opened this issue Oct 19, 2020 · 10 comments
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@Rattenkrieg
Copy link
Contributor

I'm learning the code behind devirtualization and stumbled upon this one:
https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/importer.cpp#L20539-L20547
For some reason this doesn't embeds, so here are lines under question:

        CORINFO_CLASS_HANDLE uniqueImplementingClass = NO_CLASS_HANDLE;

        // info.compCompHnd->getUniqueImplementingClass(objClass);

        if (uniqueImplementingClass == NO_CLASS_HANDLE)
        {
            JITDUMP("No unique implementor of interface %p (%s), sorry\n", dspPtr(objClass), objClassName);
            return;
        }

This was introduced in dotnet/coreclr#21270 and never changed afterwards
What my guesses are:
There is no implementation (yet) for getUniqueImplementingClass, so commented out getUniqueImplementingClass(objClass); makes sense. And I guess this stub meant to produce value for uniqueImplementingClass.
What surprised me is the following logic ending with the call to addGuardedDevirtualizationCandidate which will never be reached due to if (uniqueImplementingClass == NO_CLASS_HANDLE).

I found no mention of this in dotnet/coreclr#21270
Github search for getUniqueImplementingClass gives me nothing as well.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Oct 19, 2020
@EgorBo
Copy link
Member

EgorBo commented Oct 19, 2020

As far as I understand it - it's something a PGO could help with, see #43371 (namely https://github.com/dotnet/runtime/blob/9fb832835ae76aaea574ab88b23da86933b1aa15/docs/design/features/DynamicPgo.md#guarded-devirtualization)

I had a quick prototype to implement getUniqueImplementingClass based on attributes (e.g. placed by ILLink) #37563
e.g.

[__UniqueImplementation("System.Lion, Animals")] // emitted by ILLink substep
public interface IAnimal
{
    int GetLegsCount();
}
``

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 19, 2020

You're right that we never create guarded devirtualization candidates for interface calls -- it is not obvious to the jit what class it should guess, and so the jit has to rely on the runtime, and the runtime part is missing. There was some experimental runtime support for getUniqueImplementingClass , but we never committed that code.

[edit: added "never" to the above]

For virtual calls the jit can guess a plausible type and drive guarded devirtualization, and we enable that in some of our weekend testing, so the guarded devirt code still works.

Feel free to check out the changes in my experimental branch where Tier0 will profile types at virtual and interface call sites and then Tier1 enables guarded devirtualization based on those profiles. I am actively working on this as we speak. If you want experiment with the changes in my branch, you need

set COMPlus_TieredPgo=1
set COMPlus_JitClassProfiling=1
set COMPlus_JitEnableGuardedDevirtualization=1

and you may also want

set COMPlus_TC_QuickJitForLoops=1

In that branch, getUniqueImplementingClass has morphed into getLikelyClass.

If you also write out the PGO data

set COMPlus_PGODataPath=somefile.txt
set COMPlus_WritePGOData=1

I have a tool that can analyze the data file and produce reports like the following. Happy to share this out if you find it interesting.

--- static data for all sites---

      4441           total call sites
      2078 [ 46.79%] sites were not hit at runtime
      2363 [ 53.21%] sites were hit at runtime
      1905 [ 42.90%] sites were monomorphic
       341 [  7.68%] sites were bimorphic
       117 [  2.63%] sites were polymorphic
        58 [  1.31%] sites were polymorphic predictable (0.5+)
        44 [  0.99%] sites were polymorphic marginally predictable (0.3-0.5)
        15 [  0.34%] sites were truly polymorphic

   GDV would make predictions at 2348 out of 4441 sites ==> 52.87%

--- dynamic data for all sites ---

   4060755           total calls
   2491598 [ 61.36%] calls were monomorphic
    855535 [ 21.07%] calls were bimorphic
    713622 [ 17.57%] calls were polymorphic
    281501 [  6.93%] calls were polymorphic and predictable
    160438 [  3.95%] calls were polymorphic and marginally predictable
    271683 [  6.69%] calls were truly polymorphic

  GDV would correctly predict 3240186 out of 4060755 calls ==> 79.79%

   2491598 monomorphic
    512200 [ 59.87%] bimorphic
    173107 [ 61.49%] polymorphic predictable
     63281 [ 39.44%] polymorphic marginal

  GDV would likely not try and predict these polymorphic calls, though it might pay off if virtual

     56744 [ 20.89%] polymorphic remainder

@EgorBo
Copy link
Member

EgorBo commented Oct 19, 2020

@AndyAyersMS amazing! can't wait to see a PR with it 🙂

A small question, if I want to collect a profile (and save it to a file) - I should disable tier1 promotion? because I don't want to bake profile data too soon.

@AndyAyersMS
Copy link
Member

FWIW here's a bit of the output from the jit, when running the CSCBench

**** [0] GDV in System.Type:GetTypeHandleInternal():System.RuntimeTypeHandle:this, guessing for System.Type
**** [1] GDV in System.Type:GetRootElementType():System.Type:this, guessing for System.Type
**** [2] GDV in System.Type:GetRootElementType():System.Type:this, guessing for System.Type
**** [3] GDV in System.Type:get_IsInterface():bool:this, guessing for System.Type
**** [4] GDV in System.RuntimeType:IsAssignableFrom(System.Type):bool:this, guessing for System.Type
**** [5] GDV in System.RuntimeType:IsAssignableFrom(System.Type):bool:this, guessing for System.Type
**** [6] GDV in System.RuntimeType:IsAssignableFrom(System.Type):bool:this, guessing for System.Type
**** [7] GDV in System.RuntimeType:MakeGenericType(System.Type[]):System.Type:this, guessing for System.Type
**** [8] GDV in System.RuntimeType:GetGenericArguments():System.Type[]:this, guessing for System.Type
**** [9] GDV in System.RuntimeType:GetGenericArgumentsInternal():System.RuntimeType[]:this, guessing for System.Type
**** [10] GDV in System.RuntimeTypeHandle:CopyRuntimeTypeHandles(System.Type[],byref):System.IntPtr[], guessing for System.Type
**** [11] GDV in System.Collections.Generic.ComparerHelpers:CreateDefaultEqualityComparer(System.Type):System.Object, guessing for System.Type
**** [12] GDV in System.Collections.Generic.ComparerHelpers:CreateDefaultEqualityComparer(System.Type):System.Object, guessing for System.Type
**** [13] GDV in System.Collections.Generic.ComparerHelpers:CreateDefaultEqualityComparer(System.Type):System.Object, guessing for System.Type
**** [14] GDV in System.Collections.Generic.ComparerHelpers:CreateDefaultEqualityComparer(System.Type):System.Object, guessing for System.Type
**** [15] GDV in System.RuntimeType:FilterApplyMethodBase(System.Reflection.MethodBase,int,int,int,System.Type[]):bool, guessing for System.Reflection.MethodBase
**** [16] GDV in System.RuntimeType:FilterApplyMethodBase(System.Reflection.MethodBase,int,int,int,System.Type[]):bool, guessing for System.Reflection.ParameterInfo
**** [17] GDV in System.RuntimeType:FilterApplyMethodBase(System.Reflection.MethodBase,int,int,int,System.Type[]):bool, guessing for System.Reflection.ParameterInfo
**** [18] GDV in System.RuntimeType:FilterApplyMethodBase(System.Reflection.MethodBase,int,int,int,System.Type[]):bool, guessing for System.Reflection.MethodBase
**** [19] GDV in System.RuntimeType:FilterApplyMethodBase(System.Reflection.MethodBase,int,int,int,System.Type[]):bool, guessing for System.Reflection.MethodBase
**** [20] GDV in System.RuntimeType:FilterApplyMethodBase(System.Reflection.MethodBase,int,int,int,System.Type[]):bool, guessing for System.Reflection.MethodBase
**** [21] GDV in System.RuntimeType:FilterApplyMethodBase(System.Reflection.MethodBase,int,int,int,System.Type[]):bool, guessing for System.Reflection.ParameterInfo
**** [22] GDV in System.Type:get_IsValueType():bool:this, guessing for System.Type
**** [23] GDV in System.Collections.Generic.Dictionary`2[__Canon,__Canon][System.__Canon,System.__Canon]:FindValue(System.__Canon):byref:this, guessing for System.Collections.Generic.EqualityComparer`1[__Canon]
**** [24] GDV in System.Collections.Generic.Dictionary`2[__Canon,__Canon][System.__Canon,System.__Canon]:TryInsert(System.__Canon,System.__Canon,ubyte):bool:this, guessing for System.Collections.Generic.EqualityComparer`1[__Canon]

@AndyAyersMS
Copy link
Member

@EgorBo if you want to collect a full profile for an entire process lifetime for as many methods as possible, then you want something like:

set COMPlus_TieredPgo=1
set COMPlus_JitClassProfiling=1
set COMPlus_PGODataPath=somefile.txt
set COMPlus_WritePGOData=1
set COMPlus_TC_CallCountingDelayMs=10000000
set COMPlus_ReadyToRun=0
set COMPlus_TC_QuickJitForLoops=1

The data is written during normal runtime shutdown so the process has to exit normally.

For class profiles you will see things like:

classProfile iloffs 7 samples 58 entries 6 totalCount 50 virtual
class 00007FF8AC300A48 (SyntaxTokenWithTrivia) count 29
class 00007FF8AC32AC40 (SyntaxIdentifierWithTrailingTrivia) count 5
class 00007FF8AC20F280 (Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.SyntaxToken) count 4
class 00007FF8AC329D00 (SyntaxIdentifier) count 8
class 00007FF8AC3E72E8 (SyntaxTokenWithValueAndTrivia`1[Int32]) count 3
class 00007FF8AE7F8A30 (Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.SyntaxToken+SyntaxTokenWithValueAndTrivia`1[System.String]) count 1

So at this virtual site the this obj class was most frequently SyntaxTokenWithTrivia.

Note class profiles are ignored when reading profile data files as my simple profile system doesn't have a robust way of identifying classes. So the write-out support for class profiles is just there for the analysis tool.

@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Oct 19, 2020
@AndyAyersMS AndyAyersMS mentioned this issue Oct 19, 2020
54 tasks
@BruceForstall BruceForstall added this to the 6.0.0 milestone Oct 20, 2020
@AndyAyersMS
Copy link
Member

FYI: I broke something in a recent edit so getLikelyClass is currently not working. Will try to get it fixed soon.

@AndyAyersMS
Copy link
Member

Fixed...

@EgorBo
Copy link
Member

EgorBo commented Oct 20, 2020

Looks amazing!

image

@AndyAyersMS
Copy link
Member

Glad it's working.

Have been looking about for realistic benchmarks to try this on.

@BruceForstall
Copy link
Member

Since the original question appears to be answered, I'm closing this.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

No branches or pull requests

5 participants