dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

JIT sometimes doesn't inline calls via initonly static ref typed fields. #12004

Closed omariom closed 4 years ago

omariom commented 5 years ago

Thanks to this change JIT can now devirtualize and inline virtual calls when it finds that a static readonly field has already been intialized with a reference to an instance of a sealed derived class. Like here

private static readonly Comparer<T> cmpr = CreateComparer();

private void HappenedToBeJittedFirst(T x, T y) => eqCmpr.Equals(x, y);

private void LuckyOne(T x, T y) =>  eqCmpr.Equals(x, y);

But if the field is hidden behind a get only property, JIT inlines the getter and devirtualizes the call but fails to inline it.

The repro:

[MethodImpl(MethodImplOptions.NoInlining)]
public static int CompareInts(int x, int y) => ComparerT<int>.Default.Compare(x, y);

class ComparerT<T>
{
    public static readonly Comparer<T> Default = CreateDefaultComparer();
    //public static Comparer<T> Default { get; } = CreateDefaultComparer();

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static Comparer<T> CreateDefaultComparer()
    {
        return (Comparer<T>)Activator.CreateInstance(typeof(MyGenericComparer<>).MakeGenericType(typeof(T)));
    }
}

sealed partial class MyGenericComparer<T> : Comparer<T> where T : IComparable<T>
{
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public override int Compare(T x, T y)
    {
        if (x != null)
        {
            if (y != null) return x.CompareTo(y);
            return 1;
        }
        if (y != null) return -1;
        return 0;
    }

    public override bool Equals(object obj) => obj != null && GetType() == obj.GetType();
    public override int GetHashCode() => GetType().GetHashCode();
}

This is CompareInts when it uses the field. ComparerT<int>.Default.Compare(x, y) is replaced with the body of int.CompareTo:

       0F1F440000           nop      

G_M49158_IG02:
       8BC1                 mov      eax, ecx
       3BC2                 cmp      eax, edx
       7D07                 jge      SHORT G_M49158_IG03
       B9FFFFFFFF           mov      ecx, -1
       EB0D                 jmp      SHORT G_M49158_IG05

G_M49158_IG03:
       3BC2                 cmp      eax, edx
       7E07                 jle      SHORT G_M49158_IG04
       B901000000           mov      ecx, 1
       EB02                 jmp      SHORT G_M49158_IG05

G_M49158_IG04:
       33C9                 xor      ecx, ecx

G_M49158_IG05:
       8BC1                 mov      eax, ecx

G_M49158_IG06:
       C3                   ret 

When Default is a property. The call is static, but not inlined.

       4883EC28             sub      rsp, 40
       90                   nop      
       448BC1               mov      r8d, ecx
       8BC2                 mov      eax, edx

G_M49157_IG02:
       48B9B02BCB3047010000 mov      rcx, 0x14730CB2BB0
       488B09               mov      rcx, gword ptr [rcx]
       418BD0               mov      edx, r8d
       448BC0               mov      r8d, eax
       E8DEFDFFFF           call     MyGenericComparer`1:Compare(int,int):int:this
       90                   nop      

G_M49157_IG03:
       4883C428             add      rsp, 40
       C3                   ret 

Seems like attempt to inline devirtualized call happens too late.

@AndyAyersMS, is it a known issue?

AndyAyersMS commented 5 years ago

When the jit propagates type information "up" from an inlined callee to a virtual call site in a caller, and this leads to devirtualization (aka "late" devirtualization), then the devirtualized call can't yet be inlined.

This is mentioned as one of the future enhancements for devirtualization in dotnet/runtime#7541, and also noted for a very similar example in dotnet/coreclr#20886.

Would sure like to get this working someday... it is unfortunately not a simple change.

omariom commented 5 years ago

and also noted for a very similar example in dotnet/coreclr#20886.

Ah.. just noticed that.