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.

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

Use LinkContext caching when resolving ExportedTypes #3075

Merged
merged 5 commits into from
Oct 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/linker/BannedSymbols.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
T:Mono.Cecil.Cil.ILProcessor;Use LinkerILProcessor instead
M:Mono.Cecil.TypeReference.Resolve();Use LinkContext.Resolve and LinkContext.TryResolve helpers instead
M:Mono.Cecil.MethodReference.Resolve();Use LinkContext.Resolve and LinkContext.TryResolve helpers instead
M:Mono.Cecil.ExportedType.Resolve();Use LinkContext.Resolve and LinkContext.TryResolve helpers instead
P:Mono.Collections.Generic.Collection`1{Mono.Cecil.ParameterDefinition}.Item(System.Int32); use x
P:Mono.Cecil.ParameterDefinitionCollection.Item(System.Int32); use x
2 changes: 1 addition & 1 deletion src/linker/Linker.Steps/CodeRewriterStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ MethodBody CreateStubBody (MethodDefinition method)
if (baseType is null)
return body;

MethodReference base_ctor = baseType.GetDefaultInstanceConstructor ();
MethodReference base_ctor = baseType.GetDefaultInstanceConstructor (Context);
if (base_ctor == null)
throw new NotSupportedException ($"Cannot replace constructor for '{method.DeclaringType}' when no base default constructor exists");

Expand Down
2 changes: 1 addition & 1 deletion src/linker/Linker.Steps/MarkExportedTypesTargetStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ static void InitializeExportedType (ExportedType exportedType, LinkContext conte
if (!context.Annotations.TryGetPreservedMembers (exportedType, out TypePreserveMembers members))
return;

TypeDefinition type = exportedType.Resolve ();
TypeDefinition? type = context.TryResolve (exportedType);
if (type == null) {
if (!context.IgnoreUnresolved)
context.LogError (null, DiagnosticId.ExportedTypeCannotBeResolved, exportedType.Name);
Expand Down
2 changes: 1 addition & 1 deletion src/linker/Linker.Steps/ProcessLinkerXmlBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ protected virtual void ProcessTypes (AssemblyDefinition assembly, XPathNavigator
}
}

protected virtual TypeDefinition? ProcessExportedType (ExportedType exported, AssemblyDefinition assembly, XPathNavigator nav) => exported.Resolve ();
protected virtual TypeDefinition? ProcessExportedType (ExportedType exported, AssemblyDefinition assembly, XPathNavigator nav) => _context.TryResolve (exported);

void MatchType (TypeDefinition type, Regex regex, XPathNavigator nav)
{
Expand Down
10 changes: 7 additions & 3 deletions src/linker/Linker.Steps/SweepStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -460,10 +460,12 @@ void SweepOverrides (MethodDefinition method)
// ov is in a `link` scope and is unmarked
// ShouldRemove returns true if the method is unmarked, but we also We need to make sure the override is in a link scope.
// Only things in a link scope are marked, so ShouldRemove is only valid for items in a `link` scope.
#pragma warning disable RS0030 // Cecil's Resolve is banned - it's necessary when the metadata graph isn't stable
if (method.Overrides[i].Resolve () is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov)))
method.Overrides.RemoveAt (i);
else
i++;
#pragma warning restore RS0030
}
}

Expand Down Expand Up @@ -603,9 +605,9 @@ protected override void ProcessTypeReference (TypeReference type)
// But the cache doesn't know that, it would still "resolve" the type-ref to now defunct type-def.
// For this reason we can't use the context resolution here, and must force Cecil to perform
// real type resolution again (since it can fail, and that's OK).
#pragma warning disable RS0030 // Do not used banned APIs
#pragma warning disable RS0030 // Cecil's Resolve is banned -- it's necessary when the metadata graph isn't stable
TypeDefinition td = type.Resolve ();
#pragma warning restore RS0030 // Do not used banned APIs
#pragma warning restore RS0030
if (td == null) {
//
// This can happen when not all assembly refences were provided and we
Expand All @@ -627,7 +629,9 @@ protected override void ProcessTypeReference (TypeReference type)

protected override void ProcessExportedType (ExportedType exportedType)
{
TypeDefinition td = exportedType.Resolve ();
#pragma warning disable RS0030 // Cecil's Resolve is banned -- it's necessary when the metadata graph is unstable
TypeDefinition? td = exportedType.Resolve ();
marek-safar marked this conversation as resolved.
Show resolved Hide resolved
#pragma warning restore RS0030
if (td == null) {
// Forwarded type cannot be resolved but it was marked
// linker is running in --skip-unresolved true mode
Expand Down
58 changes: 58 additions & 0 deletions src/linker/Linker/LinkContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,11 @@ public int GetTargetRuntimeVersion ()
readonly Dictionary<MethodReference, MethodDefinition?> methodresolveCache = new ();
readonly Dictionary<FieldReference, FieldDefinition?> fieldresolveCache = new ();
readonly Dictionary<TypeReference, TypeDefinition?> typeresolveCache = new ();
readonly Dictionary<ExportedType, TypeDefinition?> exportedTypeResolveCache = new ();

/// <summary>
/// Tries to resolve the MethodReference to a MethodDefinition and logs a warning if it can't
/// </summary>
public MethodDefinition? Resolve (MethodReference methodReference)
{
if (methodReference is MethodDefinition methodDefinition)
Expand All @@ -765,14 +769,19 @@ public int GetTargetRuntimeVersion ()
if (methodresolveCache.TryGetValue (methodReference, out MethodDefinition? md))
return md;

#pragma warning disable RS0030 // Cecil's resolve is banned -- this provides the wrapper
md = methodReference.Resolve ();
#pragma warning restore RS0030
if (md == null && !IgnoreUnresolved)
ReportUnresolved (methodReference);

methodresolveCache.Add (methodReference, md);
return md;
}

/// <summary>
/// Tries to resolve the MethodReference to a MethodDefinition and returns null if it can't
/// </summary>
public MethodDefinition? TryResolve (MethodReference methodReference)
{
if (methodReference is MethodDefinition methodDefinition)
Expand All @@ -784,11 +793,16 @@ public int GetTargetRuntimeVersion ()
if (methodresolveCache.TryGetValue (methodReference, out MethodDefinition? md))
return md;

#pragma warning disable RS0030 // Cecil's resolve is banned -- this method provides the wrapper
md = methodReference.Resolve ();
#pragma warning restore RS0030
methodresolveCache.Add (methodReference, md);
return md;
}

/// <summary>
/// Tries to resolve the FieldReference to a FieldDefinition and logs a warning if it can't
/// </summary>
public FieldDefinition? Resolve (FieldReference fieldReference)
{
if (fieldReference is FieldDefinition fieldDefinition)
Expand All @@ -808,6 +822,9 @@ public int GetTargetRuntimeVersion ()
return fd;
}

/// <summary>
/// Tries to resolve the FieldReference to a FieldDefinition and returns null if it can't
/// </summary>
public FieldDefinition? TryResolve (FieldReference fieldReference)
{
if (fieldReference is FieldDefinition fieldDefinition)
Expand All @@ -824,6 +841,9 @@ public int GetTargetRuntimeVersion ()
return fd;
}

/// <summary>
/// Tries to resolve the TypeReference to a TypeDefinition and logs a warning if it can't
/// </summary>
public TypeDefinition? Resolve (TypeReference typeReference)
{
if (typeReference is TypeDefinition typeDefinition)
Expand Down Expand Up @@ -851,6 +871,9 @@ public int GetTargetRuntimeVersion ()
return td;
}

/// <summary>
/// Tries to resolve the TypeReference to a TypeDefinition and returns null if it can't
/// </summary>
public TypeDefinition? TryResolve (TypeReference typeReference)
{
if (typeReference is TypeDefinition typeDefinition)
Expand Down Expand Up @@ -881,6 +904,33 @@ public int GetTargetRuntimeVersion ()
return td;
}

/// <summary>
/// Tries to resolve the ExportedType to a TypeDefinition and logs a warning if it can't
/// </summary>
public TypeDefinition? Resolve (ExportedType et)
{
if (TryResolve (et) is not TypeDefinition td) {
ReportUnresolved (et);
return null;
}
return td;
}

/// <summary>
/// Tries to resolve the ExportedType to a TypeDefinition and returns null if it can't
/// </summary>
public TypeDefinition? TryResolve (ExportedType et)
{
if (exportedTypeResolveCache.TryGetValue (et, out var td)) {
return td;
}
#pragma warning disable RS0030 // Cecil's Resolve is banned -- this method provides the wrapper
td = et.Resolve ();
#pragma warning restore RS0030
exportedTypeResolveCache.Add (et, td);
return td;
}

public TypeDefinition? TryResolve (AssemblyDefinition assembly, string typeNameString)
{
// It could be cached if it shows up on fast path
Expand All @@ -891,6 +941,8 @@ public int GetTargetRuntimeVersion ()

readonly HashSet<MemberReference> unresolved_reported = new ();

readonly HashSet<ExportedType> unresolved_exported_types_reported = new ();

protected virtual void ReportUnresolved (FieldReference fieldReference)
{
if (unresolved_reported.Add (fieldReference))
Expand All @@ -908,6 +960,12 @@ protected virtual void ReportUnresolved (TypeReference typeReference)
if (unresolved_reported.Add (typeReference))
LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, typeReference.GetDisplayName ()), (int) DiagnosticId.FailedToResolveMetadataElement);
}

protected virtual void ReportUnresolved (ExportedType et)
{
if (unresolved_exported_types_reported.Add (et))
LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, et.Name), (int) DiagnosticId.FailedToResolveMetadataElement);
}
}

public class CodeOptimizationsSettings
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker/MarkingHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public void MarkMatchingExportedType (TypeDefinition typeToMatch, AssemblyDefini
if (typeToMatch == null || assembly == null)
return;

if (assembly.MainModule.GetMatchingExportedType (typeToMatch, out var exportedType))
if (assembly.MainModule.GetMatchingExportedType (typeToMatch, _context, out var exportedType))
MarkExportedType (exportedType, assembly.MainModule, reason, origin);
}

Expand All @@ -40,7 +40,7 @@ public void MarkForwardedScope (TypeReference typeReference, in MessageOrigin or
var assembly = _context.Resolve (typeReference.Scope);
if (assembly != null &&
_context.TryResolve (typeReference) is TypeDefinition typeDefinition &&
assembly.MainModule.GetMatchingExportedType (typeDefinition, out var exportedType))
assembly.MainModule.GetMatchingExportedType (typeDefinition, _context, out var exportedType))
MarkExportedType (exportedType, assembly.MainModule, new DependencyInfo (DependencyKind.ExportedType, typeReference), origin);
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/linker/Linker/MethodReferenceExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ public static string GetDisplayName (this MethodReference method)
var sb = new System.Text.StringBuilder ();

// Match C# syntaxis name if setter or getter
#pragma warning disable RS0030 // Cecil's Resolve is banned -- this should be a very cold path and makes calling this method much simpler
var methodDefinition = method.Resolve ();
#pragma warning restore RS0030
if (methodDefinition != null && (methodDefinition.IsSetter || methodDefinition.IsGetter)) {
// Append property name
string name = methodDefinition.IsSetter ? string.Concat (methodDefinition.Name.AsSpan (4), ".set") : string.Concat (methodDefinition.Name.AsSpan (4), ".get");
Expand Down
7 changes: 4 additions & 3 deletions src/linker/Linker/ModuleDefinitionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,18 @@ public static bool IsCrossgened (this ModuleDefinition module)
(module.Attributes & ModuleAttributes.ILLibrary) != 0;
}

public static bool GetMatchingExportedType (this ModuleDefinition module, TypeDefinition typeDefinition, [NotNullWhen (true)] out ExportedType? exportedType)
public static bool GetMatchingExportedType (this ModuleDefinition module, TypeDefinition typeDefinition, LinkContext context, [NotNullWhen (true)] out ExportedType? exportedType)
{
exportedType = null;
if (!module.HasExportedTypes)
return false;

foreach (var et in module.ExportedTypes)
if (et.Resolve () == typeDefinition) {
foreach (var et in module.ExportedTypes) {
if (context.TryResolve (et) == typeDefinition) {
exportedType = et;
return true;
}
}

return false;
}
Expand Down
10 changes: 5 additions & 5 deletions src/linker/Linker/TypeReferenceExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -305,28 +305,28 @@ public static string ToCecilName (this string fullTypeName)
return fullTypeName.Replace ('+', '/');
}

public static bool HasDefaultConstructor (this TypeDefinition type)
public static bool HasDefaultConstructor (this TypeDefinition type, LinkContext context)
{
foreach (var m in type.Methods) {
if (m.HasParameters)
continue;

var definition = m.Resolve ();
var definition = context.Resolve (m);
if (definition?.IsDefaultConstructor () == true)
return true;
}

return false;
}

public static MethodReference GetDefaultInstanceConstructor (this TypeDefinition type)
public static MethodReference GetDefaultInstanceConstructor (this TypeDefinition type, LinkContext context)
{
foreach (var m in type.Methods) {
if (m.HasParameters)
continue;

var definition = m.Resolve ();
if (!definition.IsDefaultConstructor ())
var definition = context.Resolve (m);
if (definition?.IsDefaultConstructor () != true)
continue;

return m;
Expand Down