Improve performance of interface method resolution in ILC #103066

filipnavara · 2024-06-05T07:55:17Z

Reverse order of loop in MightHaveInterfaceDispatchMap
Presumably the last interfaces on the list are more likely to be coming from the less nested classes and allow short circuiting the loop earlier.
Short-circuit recursive ResolveInterfaceMethodToVirtualMethodOnType
When ResolveInterfaceMethodToVirtualMethodOnType is recursively calling itself, use the result instead of returning null and then continuing the outer loop that is effectively doing the same recursion on base types.

Presumably the last interfaces on the list are more likely to be coming from the less nested classes and allow short circuiting the loop earlier.

When ResolveInterfaceMethodToVirtualMethodOnType is recursively calling itself, use the result instead of returning `null` and then continuing the outer loop that is effectively doing the same recursion on base types.

filipnavara · 2024-07-04T11:44:02Z

@dotnet/ilc-contrib anyone?

MichalStrehovsky · 2024-07-09T09:20:24Z

@dotnet/ilc-contrib anyone?

Like I wrote in #103034 (comment). One change is straightforward and I can easily sign off on it. For the other one you'd need @davidwrighton to look at it. If you want to get traction on this, I would suggest splitting this PR.

filipnavara · 2024-07-09T10:45:57Z

I can split it... but arguably the other change is just as trivial.

The added parameter ResolveInterfaceMethodToVirtualMethodOnType is only set to true in one loop that walks up the class hierarchy and it only skips taking the exact same code path twice (which is recursive, hence the exponential behavior).

agocke · 2024-07-04T14:51:56Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs

@@ -729,7 +729,7 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnTypeRecursive(M
                    return null;
                }

-                MethodDesc currentTypeInterfaceResolution = ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod, currentType);
+                MethodDesc currentTypeInterfaceResolution = ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod, currentType, returnRecursive: true);
                if (currentTypeInterfaceResolution != null)


When can this be null now?

There's still one branch that can return null in theory:

runtime/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs

Line 659 in 9b20327

return null;

I don't think it can happen in practice since it's likely impossible code path in the grand scheme of things but I didn't feel safe enough to reason about it.

And if it does we go back through the loop with the base type… but didn’t we already do that in the first call? Is there a way this could return anything except null?

And if it does we go back through the loop with the base type… but didn’t we already do that in the first call?

That's precisely what this PR fixes. If we call ResolveInterfaceMethodToVirtualMethodOnType recursively from inside ResolveInterfaceMethodToVirtualMethodOnTypeRecursive we just return and use the result instead of throwing it away (ie. returning null) and then going to the base type (next step of the outer loop) and computing the exact same thing again.

--

There are three cases when ResolveInterfaceMethodToVirtualMethodOnType returns null:

if (currentType.IsInterface)

if (!IsInterfaceImplementedOnType(currentType, interfaceType))

MethodDesc baseClassImplementationOfInterfaceMethod = ResolveInterfaceMethodToVirtualMethodOnTypeRecursive(interfaceMethod, baseType);

Presumably, 1. can only happen once and will never walk the type hierarchy through the base type. I didn't study the case 2. in detail but it should not matter, worst case it ends up doing the same thing it did now. Case 3. is the one I am trying to optimize.

If we are already in the ResolveInterfaceMethodToVirtualMethodOnTypeRecursive loop then returning null at 3. would continue the loop and proceed to base type. That's exactly the same thing we just computed by calling ResolveInterfaceMethodToVirtualMethodOnTypeRecursive(interfaceMethod, baseType); though, so instead of returning null in the case (and specifically only in this case, hence the added parameter) just return the computed value and short-circuit the outer loop.

Or, put another way, the returnRecursive: true indicates that if x = ResolveInterfaceMethodToVirtualMethodOnTypeRecursive(interfaceMethod, baseType); is computed AND x is not null then it changes the behavior to return x; instead of return null.

Previously it returned null, the loop proceeded to set currentType = currentType.MetadataBaseType; and continuing the loop would be equivalent to executing the recursive form return ResolveInterfaceMethodToVirtualMethodOnTypeRecursive(interfaceMethod, currentType.MetadataBaseType);.

The observation is that we just computed it and threw it away... so let's not throw it away.

agocke · 2024-07-09T15:06:35Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs

@@ -729,7 +729,7 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnTypeRecursive(M
                    return null;
                }

-                MethodDesc currentTypeInterfaceResolution = ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod, currentType);
+                MethodDesc currentTypeInterfaceResolution = ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod, currentType, returnRecursive: true);
                if (currentTypeInterfaceResolution != null)


And if it does we go back through the loop with the base type… but didn’t we already do that in the first call? Is there a way this could return anything except null?

agocke · 2024-07-09T15:15:12Z

...oreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceDispatchMapNode.cs

@@ -89,7 +89,7 @@ public static bool MightHaveInterfaceDispatchMap(TypeDesc type, NodeFactory fact

            DefType declType = type.GetClosestDefType();

-            for (int interfaceIndex = 0; interfaceIndex < declType.RuntimeInterfaces.Length; interfaceIndex++)
+            for (int interfaceIndex = declType.RuntimeInterfaces.Length - 1; interfaceIndex >= 0; interfaceIndex--)


Since this method is only returning a Boolean I think this is equivalent, but could you ensure that there’s no possible condition in which this could change the result? I think what I want to know is the invariants that need to hold for the same behavior to occur. Something like: the search returns true if any of the items are true, and checks on earlier items don’t affect the checks on later items (each check is independent) so this effectively forms a Contains call that is order invariant. But I don’t if that’s true (the code/type system is complex).

I think this is quite trivial to reason about. There's no return false or break inside the loop. All the called methods are deterministic/idempotent, and there's no local state that is modified in the loop. Hence, the only thing that can short circuit the loop is one of the return true calls. That cannot affect the returned result, it can only happen sooner or later (and the assumption of the optimization is that it's more likely to hit the condition sooner and avoid extra work).

agocke · 2024-10-21T20:10:01Z

@jkotas @davidwrighton any opinions on the riskiness of this change? I'm inclined to take it for .NET 10. I'd appreciate your thoughts.

agocke · 2024-10-21T20:10:24Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-10-21T20:10:37Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2024-10-22T05:04:34Z

any opinions on the riskiness of this change

Same opinion as what @MichalStrehovsky said above - @davidwrighton needs to review the second change.

davidwrighton

@agocke thank you for assigning this to me, and sorry for the delay @filipnavara. I've spent most of the fall dealing with a Microsoft internal issue. Thankfully, this change isn't in the brain-melting complicated part of virtual resolution, and I believe this is correct, and is probably a nice performance optimization that should improve things nicely for all sorts of customers.

However, I'd like a comment to describe the use fo the returnRecursive switch, describing the conditions under which it is used, which makes it clear that it is only for use when being called from something like ResolveInterfaceMethodToVirtualMethodOnTypeRecursive

filipnavara · 2025-01-15T19:53:43Z

However, I'd like a comment to describe the use of the returnRecursive switch...

I'll try to add the comment tomorrow. I need to refresh my memory a bit and think about how to word it.

* main: (89 commits) Add Dispose for X509Chain instance (dotnet#110740) Fix XML comment on regex split enumerator (dotnet#111572) JIT: tolerate missing InitClass map in SPMI (dotnet#111555) Build ilasm/ildasm packages for the host machine (dotnet#111512) Unicode 16.0 Support (dotnet#111469) Improve performance of interface method resolution in ILC (dotnet#103066) Fix building the host-targeting components and packing ILC (dotnet#111552) Improve JSON validation perf (dotnet#111332) Update github-merge-flow.jsonc to autoflow 9.0 to 9.0-staging (dotnet#111549) Include GPL-3 licence text in the notice (dotnet#111528) Remove explicit __compact_unwind entries from x64 assembler (dotnet#111530) Add MemoryExtensions overloads with comparer (dotnet#110197) Avoid capturing the ExecutionContext for the whole HTTP connection lifetime (dotnet#111475) Forward DefaultArtifactVisibility down from the VMR orchestrator (dotnet#111513) Fix relocs errors on riscv64 (dotnet#111317) Added JITDUMP_USE_ARCH_TIMESTAMP support. (dotnet#111359) add rcl/rcr tp and latency info (dotnet#111442) Fix stack overflow in compiler-generated state (dotnet#109207) Produce a package with the host-running ILC for repos in the VMR (dotnet#111443) Delete dead code in ilasm PE writer (dotnet#111218) ...

Reverse order of loop in MightHaveInterfaceDispatchMap

924c611

Presumably the last interfaces on the list are more likely to be coming from the less nested classes and allow short circuiting the loop earlier.

ghost added the area-NativeAOT-coreclr label Jun 5, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 5, 2024

Short-circuit recursive ResolveInterfaceMethodToVirtualMethodOnType

9b20327

When ResolveInterfaceMethodToVirtualMethodOnType is recursively calling itself, use the result instead of returning `null` and then continuing the outer loop that is effectively doing the same recursion on base types.

filipnavara force-pushed the ivm-resolution branch from 17707f6 to 9b20327 Compare June 5, 2024 08:13

filipnavara marked this pull request as ready for review June 5, 2024 09:26

filipnavara requested a review from MichalStrehovsky as a code owner June 5, 2024 09:26

build-analysis bot mentioned this pull request Jun 5, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Open

agocke reviewed Jul 9, 2024

View reviewed changes

filipnavara requested a review from davidwrighton August 16, 2024 07:24

agocke requested a review from jkotas October 21, 2024 20:09

Merge branch 'main' into ivm-resolution

e451ce7

agocke assigned davidwrighton Jan 13, 2025

davidwrighton requested changes Jan 15, 2025

View reviewed changes

Add comment

72a2514

davidwrighton approved these changes Jan 18, 2025

View reviewed changes

davidwrighton merged commit dd76387 into dotnet:main Jan 18, 2025
89 checks passed

github-actions bot locked and limited conversation to collaborators Feb 17, 2025

filipnavara deleted the ivm-resolution branch April 2, 2025 20:07

Improve performance of interface method resolution in ILC #103066

Improve performance of interface method resolution in ILC #103066

Uh oh!

Conversation

filipnavara commented Jun 5, 2024

Uh oh!

filipnavara commented Jul 4, 2024

Uh oh!

MichalStrehovsky commented Jul 9, 2024

Uh oh!

filipnavara commented Jul 9, 2024

Uh oh!

agocke Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

filipnavara Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

agocke Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

filipnavara Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

filipnavara Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

agocke Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

agocke Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke commented Oct 21, 2024

Uh oh!

agocke commented Oct 21, 2024

Uh oh!

azure-pipelines bot commented Oct 21, 2024

Uh oh!

jkotas commented Oct 22, 2024

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

filipnavara commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

agocke Jul 9, 2024 •

edited

Loading

filipnavara Jul 9, 2024 •

edited

Loading