Skip to content

Improve performance on virtual method processing #100897

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 8 commits into from
Apr 12, 2024
Merged

Improve performance on virtual method processing #100897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ba8b585
Improve performance on virtual method processing
jtschuster Apr 11, 2024
08ec419
Merge branch 'main' of https://github.com/dotnet/runtime into Trimmer…
jtschuster Apr 11, 2024
165384d
Undo recursive interface check with behavior change
jtschuster Apr 11, 2024
587a829
Undo test changes
jtschuster Apr 11, 2024
e58abc3
Merge branch 'main' into TrimmerRegression
jtschuster Apr 11, 2024
1edfcda
Make method internal for API compat
jtschuster Apr 11, 2024
62eac03
Merge branch 'TrimmerRegression' of https://github.com/jtschuster/run…
jtschuster Apr 11, 2024
a811131
Make GetOverrides use IEnumerable and cast to List
jtschuster Apr 12, 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
21 changes: 10 additions & 11 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ protected LinkContext Context {
}

protected Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> _methods;
protected HashSet<(MethodDefinition, MarkScopeStack.Scope)> _virtual_methods;
protected Dictionary<MethodDefinition, MarkScopeStack.Scope> _virtual_methods;
protected Queue<AttributeProviderPair> _assemblyLevelAttributes;
readonly List<AttributeProviderPair> _ivt_attributes;
protected Queue<(AttributeProviderPair, DependencyInfo, MarkScopeStack.Scope)> _lateMarkedAttributes;
Expand Down Expand Up @@ -220,7 +220,7 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH
public MarkStep ()
{
_methods = new Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> ();
_virtual_methods = new HashSet<(MethodDefinition, MarkScopeStack.Scope)> ();
_virtual_methods = new Dictionary<MethodDefinition, MarkScopeStack.Scope> ();
Copy link
Member

Choose a reason for hiding this comment

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

Is the perf improvement due to the change of key? If so is MethodDefinition actually a good option for lookups? It doesn't implement IEquatable and doesn't have a custom GetHashcode. Would a custom record using what makes a MethodDefinition identity better then?

Copy link
Member Author

Choose a reason for hiding this comment

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

The trimmer only creates a single MethodDefinition object for each method definition in metadata, so object's implementation of GetHashCode and Equals are fine here.

_assemblyLevelAttributes = new Queue<AttributeProviderPair> ();
_ivt_attributes = new List<AttributeProviderPair> ();
_lateMarkedAttributes = new Queue<(AttributeProviderPair, DependencyInfo, MarkScopeStack.Scope)> ();
Expand Down Expand Up @@ -588,10 +588,8 @@ void ProcessMarkedTypesWithInterfaces ()
// We may mark an interface type later on. Which means we need to reprocess any time with one or more interface implementations that have not been marked
// and if an interface type is found to be marked and implementation is not marked, then we need to mark that implementation

// copy the data to avoid modified while enumerating error potential, which can happen under certain conditions.
var typesWithInterfaces = _typesWithInterfaces.ToArray ();

foreach ((var type, var scope) in typesWithInterfaces) {
for (int i = 0; i < _typesWithInterfaces.Count; i++) {
(var type, var scope) = _typesWithInterfaces[i];
// Exception, types that have not been flagged as instantiated yet. These types may not need their interfaces even if the
// interface type is marked
// UnusedInterfaces optimization is turned off mark all interface implementations
Expand All @@ -609,7 +607,7 @@ void ProcessMarkedTypesWithInterfaces ()
continue;
foreach (var ov in baseMethods) {
if (ov.Base.DeclaringType is not null && ov.Base.DeclaringType.IsInterface && IgnoreScope (ov.Base.DeclaringType.Scope)) {
_virtual_methods.Add ((ov.Base, ScopeStack.CurrentScope));
_virtual_methods.TryAdd (ov.Base, ScopeStack.CurrentScope);
}
}
}
Expand Down Expand Up @@ -707,9 +705,10 @@ void ProcessVirtualMethod (MethodDefinition method)
MarkMethod (dimInfo.Override, new DependencyInfo (DependencyKind.Override, dimInfo.Base), ScopeStack.CurrentScope.Origin);
}
}
var overridingMethods = Annotations.GetOverrides (method);
List<OverrideInformation>? overridingMethods = (List<OverrideInformation>?)Annotations.GetOverrides (method);
if (overridingMethods is not null) {
foreach (OverrideInformation ov in overridingMethods) {
for (int i = 0; i < overridingMethods.Count; i++) {
OverrideInformation ov = overridingMethods[i];
if (IsInterfaceImplementationMethodNeededByTypeDueToInterface (ov))
MarkMethod (ov.Override, new DependencyInfo (DependencyKind.Override, ov.Base), ScopeStack.CurrentScope.Origin);
}
Expand Down Expand Up @@ -3267,7 +3266,7 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
MarkMethodSpecialCustomAttributes (method);

if (method.IsVirtual)
_virtual_methods.Add ((method, ScopeStack.CurrentScope));
_virtual_methods.TryAdd (method, ScopeStack.CurrentScope);

MarkNewCodeDependencies (method);

Expand Down Expand Up @@ -3475,7 +3474,7 @@ void MarkBaseMethods (MethodDefinition method)
// This will produce warnings for all interface methods and virtual methods regardless of whether the interface, interface implementation, or interface method is kept or not.
if (ov.Base.DeclaringType.IsInterface && !method.DeclaringType.IsInterface) {
// These are all virtual, no need to check IsVirtual before adding to list
_virtual_methods.Add ((ov.Base, ScopeStack.CurrentScope));
_virtual_methods.TryAdd (ov.Base, ScopeStack.CurrentScope);
continue;
}

Expand Down
2 changes: 1 addition & 1 deletion src/tools/illink/src/linker/Linker/Annotations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,7 @@ public void EnqueueVirtualMethod (MethodDefinition method)
VirtualMethodsWithAnnotationsToValidate.Add (method);
}

internal List<(TypeReference, List<InterfaceImplementation>)>? GetRecursiveInterfaces (TypeDefinition type)
internal List<(TypeReference InterfaceType, List<InterfaceImplementation> ImplementationChain)>? GetRecursiveInterfaces (TypeDefinition type)
{
return TypeMapInfo.GetRecursiveInterfaces (type);
}
Expand Down
7 changes: 4 additions & 3 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 @@ -66,7 +66,7 @@ public void EnsureProcessed (AssemblyDefinition assembly)
/// <summary>
/// Returns a list of all known methods that override <paramref name="method"/>. The list may be incomplete if other overrides exist in assemblies that haven't been processed by TypeMapInfo yet
/// </summary>
public IEnumerable<OverrideInformation>? GetOverrides (MethodDefinition method)
public List<OverrideInformation>? GetOverrides (MethodDefinition method)
{
EnsureProcessed (method.Module.Assembly);
override_methods.TryGetValue (method, out List<OverrideInformation>? overrides);
Expand Down Expand Up @@ -130,14 +130,15 @@ protected virtual void MapType (TypeDefinition type)
MapType (nested);
}

internal List<(TypeReference, List<InterfaceImplementation>)>? GetRecursiveInterfaces (TypeDefinition type)
internal List<(TypeReference InterfaceType, List<InterfaceImplementation> ImplementationChain)>? GetRecursiveInterfaces (TypeDefinition type)
{
EnsureProcessed(type.Module.Assembly);
if (interfaces.TryGetValue (type, out var value))
return value;
return null;
}

List<(TypeReference, List<InterfaceImplementation>)> GetRecursiveInterfaceImplementations (TypeDefinition type)
List<(TypeReference InterfaceType, List<InterfaceImplementation> ImplementationChain)> GetRecursiveInterfaceImplementations (TypeDefinition type)
{
List<(TypeReference, List<InterfaceImplementation>)> firstImplementationChain = new ();

Expand Down