[ILLlink] In DIM finding, exit early if the interface doesn't impl the DIM we're looking for

[jtschuster]

When finding default interface methods, we search all methods on all interfaces a type implements. We can narrow that search to only interfaces that implement the type that has the interface method we are looking for. This method was one of the most expensive in a hello world, but this change cuts this methods execution time in half.

[ghost]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When finding default interface methods, we search all methods on all interfaces a type implements. We can narrow that search to only interfaces that implement the type that has the interface method we are looking for. This method was one of the most expensive in a hello world, but this change cuts this methods execution time in half.

Author:	jtschuster
Assignees:	-
Labels:	area-Tools-ILLink
Milestone:	-

[agocke]

Is this affected by IDynamicInterfaceCastable? That is, can it contribute to this lookup?

• @jkoritzinsky

[jkoritzinsky]

Based on how this code is written, I don't believe that IDIC can contribute to the lookup here.

The loop here only looks at statically-implemented interfaces on the type, so IDIC implementations weren't found in the existing implementation anyway.

[agocke]

LGTM. Note that LINQ will cause an allocation here due to the closure. If you want this to greatly improve performance you'll need a foreach loop

[MichalStrehovsky]

I left a comment at the previous PR at https://github.com/dotnet/runtime/pull/98183/files#r1490487878 because it's more relevant there but maybe the perf can be improved even more. Either that or there might be a bug.

Note that in IL, the interface doesn't need to repeat interfaces from the bases.

interface IFoo { }

interface IBar : IFoo { }

interface IBaz: IBar /*, IFoo */ { }

Roslyn will add the thing I commented out above, but IL doesn't require it and with various versioning issues, one can end up seeing this in the wild too (a Nuget package v1 shipping IBar and then later making IBar implement IFoo: default interface methods were specifically introduced to allow this).

Not sure if it's a problem for current ILLinker design, but short circuiting in this PR will probably prevent drilling into IBar to look for impls of IFoo that would previously happen here.

[agocke]

Seems like potentialImplInterface.Interfaces needs to return all (transitive) interfaces in order for this to produce the right results. I don't know what Cecil does here -- Roslyn does produce the transitive interface list.

[agocke]

You could refactor this into a local function to avoid doing the break-continue dance.

[jtschuster]

Seems like potentialImplInterface.Interfaces needs to return all (transitive) interfaces in order for this to produce the right results. I don't know what Cecil does here -- Roslyn does produce the transitive interface list.

I think Cecil only returns InterfaceImpls directly on the type

Are classes required to have impls for each interface in the interface hierarchy in IL, or is that only a C# implementation detail? If classes have to implement all interfaces, I think this should be fine for instance methods? For instance methods, we should only need to call this on classes, not interfaces, so we should always start looking at the class/struct MyFoo that implements all interfaces in the hierarchy, and even if IBaz doesn't have an impl for IBar, MyFoo will, so we should still end up looking everywhere we need to.

If classes aren't required to have impls for everything up the interface hierarchy, I think this optimization won't work.

interface IFoo
{
	void Method();
}

interface IBar : IFoo
{
	void IFoo.Method() { }
}

interface IBaz: IBar /*, IFoo */
{
	void Method() { }
}
    
class MyFoo : IBaz /* IBar, IFoo */
{ }

Static interface methods I think we do run into a problem regardless. It looks like the following code runs okay even if the interfaces only implement the one interface listed, but we wouldn't recognize that I2 provides a DIM for I4

interface IBase
{
    static abstract void Method();
}

interface I2 : IBase { static void IBase.Method() { }}

interface I3 : I2 { }

interface I4 : I3 /* but not I2 or IBase */ { }

static void CallMethod<T>() where T : IBase
{
	T.Method();
}

static void Method()
{
	CallMethod<I4>();
}

[jtschuster]

If classes aren't required to have impls for everything up the interface hierarchy, I think this optimization won't work.

Looks like this is the case, this optimization won't work.