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.

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

Don't use IEnumerable and yield return for GetDefaultInterfaceImplementations #98183

Merged
merged 3 commits into from
Feb 10, 2024
Merged

Don't use IEnumerable and yield return for GetDefaultInterfaceImplementations #98183

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4878be1
Don't use IEnumerable for GetDefaultInterfaceImplementations
jtschuster Feb 8, 2024
c2ef28d
Apply suggestions from code review
jtschuster Feb 8, 2024
d0d00c8
Merge branch 'main' into SpeedUpGetDefaultImplementations
jtschuster Feb 8, 2024
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
13 changes: 5 additions & 8 deletions src/tools/illink/src/linker/Linker/TypeMapInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,7 @@ void MapInterfaceMethodsInTypeHierarchy (TypeDefinition type)
}

// Look for a default implementation last.
foreach (var defaultImpl in GetDefaultInterfaceImplementations (type, resolvedInterfaceMethod)) {
AddDefaultInterfaceImplementation (resolvedInterfaceMethod, type, defaultImpl);
}
FindAndAddDefaultInterfaceImplementations (type, resolvedInterfaceMethod);
}
}
}
Expand Down Expand Up @@ -276,7 +274,7 @@ void AnnotateMethods (MethodDefinition @base, MethodDefinition @override, Interf
// Note that this returns a list to potentially cover the diamond case (more than one
// most specific implementation of the given interface methods). ILLink needs to preserve
// all the implementations so that the proper exception can be thrown at runtime.
IEnumerable<InterfaceImplementation> GetDefaultInterfaceImplementations (TypeDefinition type, MethodDefinition interfaceMethod)
void FindAndAddDefaultInterfaceImplementations (TypeDefinition type, MethodDefinition interfaceMethod)
{
// Go over all interfaces, trying to find a method that is an explicit MethodImpl of the
// interface method in question.
Expand All @@ -290,7 +288,7 @@ IEnumerable<InterfaceImplementation> GetDefaultInterfaceImplementations (TypeDef
foreach (var potentialImplMethod in potentialImplInterface.Methods) {
if (potentialImplMethod == interfaceMethod &&
!potentialImplMethod.IsAbstract) {
yield return interfaceImpl;
AddDefaultInterfaceImplementation (interfaceMethod, type, interfaceImpl);
}

if (!potentialImplMethod.HasOverrides)
Expand All @@ -299,7 +297,7 @@ IEnumerable<InterfaceImplementation> GetDefaultInterfaceImplementations (TypeDef
// This method is an override of something. Let's see if it's the method we are looking for.
foreach (var @override in potentialImplMethod.Overrides) {
if (context.TryResolve (@override) == interfaceMethod) {
yield return interfaceImpl;
AddDefaultInterfaceImplementation (interfaceMethod, type, interfaceImpl);
foundImpl = true;
break;
}
Expand All @@ -313,8 +311,7 @@ IEnumerable<InterfaceImplementation> GetDefaultInterfaceImplementations (TypeDef
// We haven't found a MethodImpl on the current interface, but one of the interfaces
// this interface requires could still provide it.
if (!foundImpl) {
foreach (var impl in GetDefaultInterfaceImplementations (potentialImplInterface, interfaceMethod))
yield return impl;
FindAndAddDefaultInterfaceImplementations (potentialImplInterface, interfaceMethod);
Copy link
Member

Choose a reason for hiding this comment

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

This is not producing equivalent results.

Previously, we always called AddDefaultInterfaceImplementation (resolvedInterfaceMethod, type, {ThisComesFromTheAlgorithm});

Now we are calling AddDefaultInterfaceImplementation (resolvedInterfaceMethod, {ThisComesFromTheAlgorithm}, {ThisComesFromTheAlgorithm}); because at this spot type is swapped for potentialImplInterface when this recursively calls itself at this spot.

Is that a problem? Or the second parameter doesn't matter for anything? If it doesn't matter, the perf improvement in #98436 can be even better - we don't have to look for default implementation per each class - we can only look for default implementations when MapType is called for an interface and don't worry about default implementations when looking at classes at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good catch. We do need the correct 'ImplementingType' for this, so we'll have to make sure the original type is passed to AddDefaultInterfaceImplementation.

}
}
}
Expand Down