Skip to content

Commit

Permalink
Improve performance on virtual method processing (dotnet#100897)
Browse files Browse the repository at this point in the history
The _virtual_methods cache had duplicates due to the Scope item in the tuple, which made ProcessVirtualMethods do significantly more work. This PR makes the cache a dictionary, with TryAdd's instead of Add to only add each method once. These changes made caused the overrides cache in TypeMapInfo to be modified during iteration, so a for loop is used instead of a foreach loop. This should be find since the list will always be append-only. I noticed we copy _typesWithInterfaces to an array for the same reason, though since that's also append-only we can iterate with a for loop there, too.

Fixes dotnet/sdk#40060.
  • Loading branch information
jtschuster authored Apr 12, 2024
1 parent ec76487 commit 7b18be5
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 15 deletions.
21 changes: 10 additions & 11 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
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> ();
_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
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
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

0 comments on commit 7b18be5

Please sign in to comment.