Skip to content

Improve performance of interface method resolution in ILC #103066

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 18, 2025
Merged

Improve performance of interface method resolution in ILC #103066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
924c611
Reverse order of loop in MightHaveInterfaceDispatchMap
filipnavara Jun 5, 2024
9b20327
Short-circuit recursive ResolveInterfaceMethodToVirtualMethodOnType
filipnavara Jun 5, 2024
e451ce7
Merge branch 'main' into ivm-resolution
agocke Oct 21, 2024
72a2514
Add comment
filipnavara Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,18 @@ public override MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnT
// function returns null if the interface method implementation is not defined by the current type in
// the hierarchy.For variance to work correctly, this requires that interfaces be queried in correct order.
// See current interface call resolution for details on how that happens.
private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType)
//
// The returnRecursive parameter is solely intended to optimize the recursion through
// the ResolveInterfaceMethodToVirtualMethodOnTypeRecursive helper method and should not
// be used for anything else. ResolveInterfaceMethodToVirtualMethodOnTypeRecursive walks
// up the type hierarchy, calls ResolveInterfaceMethodToVirtualMethodOnType on each base
// type, and bails out on a first successful match. Since we perform the same expansion in
// the last branch of this method by calling ResolveInterfaceMethodToVirtualMethodOnTypeRecursive
// for the base type, we allow the caller to specify that such computed value should be
// returned instead of discarded. This allows short-circuiting the outer loop over type
// hierarchy and avoids unnecessary exponential algorithmic complexity of the resolution
// algorithm.
private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType, bool returnRecursive = false)
{
Debug.Assert(!interfaceMethod.Signature.IsStatic);

Expand Down Expand Up @@ -684,7 +695,7 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc
MethodDesc baseClassImplementationOfInterfaceMethod = ResolveInterfaceMethodToVirtualMethodOnTypeRecursive(interfaceMethod, baseType);
if (baseClassImplementationOfInterfaceMethod != null)
{
return null;
return returnRecursive ? baseClassImplementationOfInterfaceMethod : null;
}
else
{
Expand Down Expand Up @@ -748,7 +759,7 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnTypeRecursive(M
return null;
}

MethodDesc currentTypeInterfaceResolution = ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod, currentType);
MethodDesc currentTypeInterfaceResolution = ResolveInterfaceMethodToVirtualMethodOnType(interfaceMethod, currentType, returnRecursive: true);
if (currentTypeInterfaceResolution != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can this be null now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. if (currentType.IsInterface)
  2. if (!IsInterfaceImplementedOnType(currentType, interfaceType))
  3. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return currentTypeInterfaceResolution;

Expand Down
2 changes: 1 addition & 1 deletion ...clr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceDispatchMapNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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--)
Copy link
Member

@agocke agocke Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@filipnavara filipnavara Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

{
DefType interfaceType = declType.RuntimeInterfaces[interfaceIndex];
InstantiatedType interfaceOnDefinitionType = interfaceType.IsTypeDefinition ?
Expand Down