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

Not inlining trivial default interface method on known struct #39419

Closed
YairHalberstadt opened this issue Jul 16, 2020 · 6 comments
Closed

Not inlining trivial default interface method on known struct #39419

YairHalberstadt opened this issue Jul 16, 2020 · 6 comments
Labels
area-VM-coreclr tenet-performance Performance related issue
Milestone

Comments

@YairHalberstadt
Copy link
Contributor

YairHalberstadt commented Jul 16, 2020

Consider the following code:

using System;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;

interface IM<T>
{
    bool UseDefaultM { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => true; }
    ValueTask M(T instance) => throw new NotImplementedException("M must be implemented if UseDefaultM is false");
    static ValueTask DefaultM(T instance) { 
        Console.WriteLine("Default Behaviour");
        return default;
    }
}

struct M : IM<int> {}

public static class Program
{
    [MethodImpl(MethodImplOptions.AggressiveOptimization)]
    static void Main()
    {
        var m = new M();
        if (((IM<int>)m).UseDefaultM)
        {
            IM<int>.DefaultM(42);
        }
        else
        {
            ((IM<int>)m).M(42);
        }
    }
}

I was hoping to use this pattern to avoid boxing m, since I would have expected UseDefaultM to be inlined, therefore avoiding boxing m when it's a struct, since either the static DefaultM method will be called, or the instance of IM overrides the DIM for M in which case it doesn't require boxing. However when I look at the jit asm on sharplab, it appears this isn't happening.

EDIT
Note that this does work when IM is non-generic:

using System;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;

interface IM
{
    bool UseDefaultM { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => true; }
    ValueTask M(int instance) => throw new NotImplementedException("M must be implemented if UseDefaultM is false");
    static ValueTask DefaultM(int instance) { 
        Console.WriteLine("Default Behaviour");
        return default;
    }
}

struct M : IM {}

public static class Program
{
    [MethodImpl(MethodImplOptions.AggressiveOptimization)]
    static void Main()
    {
        var m = new M();
        if (((IM)m).UseDefaultM)
        {
            IM.DefaultM(42);
        }
        else 
        {
            ((IM)m).M(42);
        }
    }
}

The jit works out that IM.DefaultM is always called and removes both UseDefaultM and ((IM)m).M(42) from the generated asm:

Program.Main()
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: sub esp, 0x10
    L0006: xor eax, eax
    L0008: mov [ebp-8], eax
    L000b: mov [ebp-4], eax
    L000e: mov [ebp-0x10], eax
    L0011: mov [ebp-0xc], eax
    L0014: lea ecx, [ebp-0x10]
    L0017: mov edx, 0x2a
    L001c: call IM.DefaultM(Int32)
    L0021: mov esp, ebp
    L0023: pop ebp
    L0024: ret
@YairHalberstadt YairHalberstadt added the tenet-performance Performance related issue label Jul 16, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-TypeSystem-coreclr untriaged New issue has not been triaged by the area owner labels Jul 16, 2020
@MichalStrehovsky MichalStrehovsky added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-TypeSystem-coreclr labels Jul 16, 2020
@MichalStrehovsky
Copy link
Member

The type of this within the default interface implementation is IM<int>, not M (it's a reference type, not byref to a valuetype). The code generator would have to retype the this and then deal with situations where the retyping could be invalid (e.g. bool UseDefaultM(T instance) => new object[] { this }.Length > 0; - placing the this into an object[] is not possible without it being a reference type). The interface being generic doesn't make things easier either.

Default interface methods are better to be avoided in high performance code.

@YairHalberstadt
Copy link
Contributor Author

@MichalStrehovsky

It manages to inline it when IM is not generic:

using System;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;

interface IM
{
    bool UseDefaultM { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => true; }
    ValueTask M(int instance) => throw new NotImplementedException("M must be implemented if UseDefaultM is false");
    static ValueTask DefaultM(int instance) { 
        Console.WriteLine("Default Behaviour");
        return default;
    }
}

struct M : IM {}

public static class Program
{
    [MethodImpl(MethodImplOptions.AggressiveOptimization)]
    static void Main()
    {
        var m = new M();
        if (((IM)m).UseDefaultM)
        {
            IM.DefaultM(42);
        }
        else 
        {
            ((IM)m).M(42);
        }
    }
}

@MichalStrehovsky
Copy link
Member

It manages to inline it when IM is not generic:

#9588 is probably the main reason then.

@YairHalberstadt
Copy link
Contributor Author

YairHalberstadt commented Jul 16, 2020

A decent workaround is:

using System;
using System.Threading.Tasks;

interface IM
{
    bool UseDefaultM { get => true; }
}

interface IM<T> : IM
{
    ValueTask M(T instance) => throw new NotImplementedException("M must be implemented if UseDefaultM is false");
    static ValueTask DefaultM(T instance) { 
        Console.WriteLine("Default Behaviour");
        return default;
    }
}

struct M : IM<int> {}

public static class Program
{
    static void Main()
    {
        var m = new M();
        if (((IM<int>)m).UseDefaultM)
        {
            IM<int>.DefaultM(42);
        }
        else
        {
            ((IM<int>)m).M(42);
        }
    }
}

@AndyAyersMS
Copy link
Member

Right, devirtualization is failing in resolveVirtualMethodHelper.

I should plumb through failure reason reporting and pass the reason back to the jit, so it's more obvious on the jit side what is happening. Currently we just see a generic failure:

impDevirtualizeCall: Trying to devirtualize interface call:
    class for 'this' is M [exact] (attrib 20810010)
    base method is IM`1::get_UseDefaultM
--- base class is interface
--- no derived method, sorry

Going to mark this as future and relabel as a VM issue.

@AndyAyersMS AndyAyersMS added area-VM-coreclr and removed 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 Jul 16, 2020
@AndyAyersMS AndyAyersMS added this to the Future milestone Jul 16, 2020
@MichalStrehovsky
Copy link
Member

The VM side is tracked in #9588 so we can just call this a dup and close.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

4 participants