Skip to content

Commit

Permalink
Reduce the number of forced MethodTables (#79594)
Browse files Browse the repository at this point in the history
When we're doing an optimized build, we go over the whole program twice:

* First time to build a whole program view: at this point we e.g. scan the IL method bodies to see what they depend on, or analyze reflection use within the program
* The second time to do actual codegen: this time we no longer analyze reflection use and expect that the first phase would have rooted everything that's necessary from reflection perspective.

In both phases, we assume any type with a "constructed" `MethodTable` is visible to reflection because one can just call `object.GetType()` and reflect on stuff. We need to pass a list of constructed `MethodTable`s from the first phase to the second phase because some `MethodTable`s could be the result of reflection analysis and we need to make sure they're compiled.

But crucially, up until now we didn't really track which `MethodTable`s are actual reflection roots and which ones just showed up in the dependency graph because the analyzed program happened to use it. We don't actually need to pass the latter ones as roots to compilation because the compilation phase is going to figure them out if they're needed anyway and if the compilation doesn't come up with some, that's fine because one wouldn't be able to call `object.GetType` on those anyway, because they're _not actually part of the program_.

Passing all of the MethodTables we saw from scanning to compilation is actually size bloat because scanning overapproximates things (by necessity, since it doesn't have a whole program view).

In this pull request I'm introducing `ReflectedTypeNode` to model `MethodTable`s that are actual targets of reflection. Only those will get passed as roots to the compilation phase. From now on we need to be mindful of how we refer to types. If a reference to a type is a result of non-code dependency, we should use `ReflectedType` to model it.

Saves about 1.2% (32 kB) in size on a Hello World.

I'm seeing it helps in two patterns:

https://github.com/dotnet/runtime/blob/b1a2080b9ce5833802fe2d632f79de6402097f14/src/libraries/System.Private.CoreLib/src/System/Byte.cs#L1088-L1093

RyuJIT is able to eliminate the dead code in the if branch, but we were still rooting the type within the branch from the box.

The second pattern seems to be around RyuJIT devirtualizing things and preventing a box, which now eliminates the `MethodTable`.
  • Loading branch information
MichalStrehovsky committed Jan 11, 2023
1 parent 7ac3cde commit edadf5f
Show file tree
Hide file tree
Showing 21 changed files with 230 additions and 75 deletions.
Expand Up @@ -23,6 +23,7 @@ public sealed class AnalysisBasedMetadataManager : GeneratingMetadataManager, IC
{
private readonly List<ModuleDesc> _modulesWithMetadata;
private readonly List<MetadataType> _typesWithRootedCctorContext;
private readonly List<TypeDesc> _forcedTypes;

private readonly Dictionary<TypeDesc, MetadataCategory> _reflectableTypes = new Dictionary<TypeDesc, MetadataCategory>();
private readonly Dictionary<MethodDesc, MetadataCategory> _reflectableMethods = new Dictionary<MethodDesc, MetadataCategory>();
Expand All @@ -32,7 +33,7 @@ public sealed class AnalysisBasedMetadataManager : GeneratingMetadataManager, IC
public AnalysisBasedMetadataManager(CompilerTypeSystemContext typeSystemContext)
: this(typeSystemContext, new FullyBlockedMetadataBlockingPolicy(),
new FullyBlockedManifestResourceBlockingPolicy(), null, new NoStackTraceEmissionPolicy(),
new NoDynamicInvokeThunkGenerationPolicy(), Array.Empty<ModuleDesc>(),
new NoDynamicInvokeThunkGenerationPolicy(), Array.Empty<ModuleDesc>(), Array.Empty<TypeDesc>(),
Array.Empty<ReflectableEntity<TypeDesc>>(), Array.Empty<ReflectableEntity<MethodDesc>>(),
Array.Empty<ReflectableEntity<FieldDesc>>(), Array.Empty<ReflectableCustomAttribute>(),
Array.Empty<MetadataType>(), default)
Expand All @@ -47,6 +48,7 @@ public AnalysisBasedMetadataManager(CompilerTypeSystemContext typeSystemContext)
StackTraceEmissionPolicy stackTracePolicy,
DynamicInvokeThunkGenerationPolicy invokeThunkGenerationPolicy,
IEnumerable<ModuleDesc> modulesWithMetadata,
IEnumerable<TypeDesc> forcedTypes,
IEnumerable<ReflectableEntity<TypeDesc>> reflectableTypes,
IEnumerable<ReflectableEntity<MethodDesc>> reflectableMethods,
IEnumerable<ReflectableEntity<FieldDesc>> reflectableFields,
Expand All @@ -57,6 +59,7 @@ public AnalysisBasedMetadataManager(CompilerTypeSystemContext typeSystemContext)
{
_modulesWithMetadata = new List<ModuleDesc>(modulesWithMetadata);
_typesWithRootedCctorContext = new List<MetadataType>(rootedCctorContexts);
_forcedTypes = new List<TypeDesc>(forcedTypes);

foreach (var refType in reflectableTypes)
{
Expand Down Expand Up @@ -180,10 +183,9 @@ void ICompilationRootProvider.AddCompilationRoots(IRootingServiceProvider rootPr

const string reason = "Reflection";

foreach (var pair in _reflectableTypes)
foreach (var type in _forcedTypes)
{
if ((pair.Value & MetadataCategory.RuntimeMapping) != 0)
rootProvider.AddCompilationRoot(pair.Key, reason);
rootProvider.AddReflectionRoot(type, reason);
}

foreach (var pair in _reflectableMethods)
Expand Down
Expand Up @@ -341,7 +341,7 @@ public override bool HandleCall(MethodIL callingMethodBody, MethodDesc calledMet
{
if (systemTypeValue.RepresentedType.Type.IsEnum)
{
reflectionMarker.Dependencies.Add(reflectionMarker.Factory.ConstructedTypeSymbol(systemTypeValue.RepresentedType.Type.MakeArrayType()), "Enum.GetValues");
reflectionMarker.Dependencies.Add(reflectionMarker.Factory.ReflectedType(systemTypeValue.RepresentedType.Type.MakeArrayType()), "Enum.GetValues");
}
}
else
Expand Down Expand Up @@ -381,7 +381,7 @@ public override bool HandleCall(MethodIL callingMethodBody, MethodDesc calledMet
&& systemTypeValue.RepresentedType.Type.GetParameterlessConstructor() is MethodDesc ctorMethod
&& !reflectionMarker.Factory.MetadataManager.IsReflectionBlocked(ctorMethod))
{
reflectionMarker.Dependencies.Add(reflectionMarker.Factory.ReflectableMethod(ctorMethod), "Marshal API");
reflectionMarker.Dependencies.Add(reflectionMarker.Factory.ReflectedMethod(ctorMethod), "Marshal API");
}
}
}
Expand Down
Expand Up @@ -128,8 +128,8 @@ private static void AddDependenciesDueToCustomAttributes(ref DependencyList depe
// Make a new list in case we need to abort.
var caDependencies = factory.MetadataManager.GetDependenciesForCustomAttribute(factory, constructor, decodedValue, parent) ?? new DependencyList();

caDependencies.Add(factory.ReflectableMethod(constructor), "Attribute constructor");
caDependencies.Add(factory.ConstructedTypeSymbol(constructor.OwningType), "Attribute type");
caDependencies.Add(factory.ReflectedMethod(constructor), "Attribute constructor");
caDependencies.Add(factory.ReflectedType(constructor.OwningType), "Attribute type");

if (AddDependenciesFromCustomAttributeBlob(caDependencies, factory, constructor.OwningType, decodedValue))
{
Expand Down Expand Up @@ -193,7 +193,7 @@ private static bool AddDependenciesFromField(DependencyList dependencies, NodeFa
if (factory.MetadataManager.IsReflectionBlocked(field))
return false;

dependencies.Add(factory.ReflectableField(field), "Custom attribute blob");
dependencies.Add(factory.ReflectedField(field), "Custom attribute blob");

return true;
}
Expand Down Expand Up @@ -234,7 +234,7 @@ private static bool AddDependenciesFromPropertySetter(DependencyList dependencie
setterMethod = factory.TypeSystemContext.GetMethodForInstantiatedType(setterMethod, (InstantiatedType)attributeType);
}

dependencies.Add(factory.ReflectableMethod(setterMethod), "Custom attribute blob");
dependencies.Add(factory.ReflectedMethod(setterMethod), "Custom attribute blob");
}

return true;
Expand All @@ -259,7 +259,7 @@ private static bool AddDependenciesFromCustomAttributeArgument(DependencyList de

// Reflection will need to be able to allocate this type at runtime
// (e.g. this could be an array that needs to be allocated, or an enum that needs to be boxed).
dependencies.Add(factory.ConstructedTypeSymbol(type), "Custom attribute blob");
dependencies.Add(factory.ReflectedType(type), "Custom attribute blob");

if (type.UnderlyingType.IsPrimitive || type.IsString || value == null)
return true;
Expand Down
Expand Up @@ -14,6 +14,9 @@ namespace ILCompiler.DependencyAnalysis
{
/// <summary>
/// Represents a field that has metadata generated in the current compilation.
/// This corresponds to a ECMA-335 FieldDef record. It is however not a 1:1
/// mapping because a field could be used in the AOT compiled program without generating
/// the reflection metadata for it (which would not be possible in IL terms).
/// </summary>
/// <remarks>
/// Only expected to be used during ILScanning when scanning for reflection.
Expand Down
Expand Up @@ -14,6 +14,9 @@ namespace ILCompiler.DependencyAnalysis
{
/// <summary>
/// Represents a method that has metadata generated in the current compilation.
/// This corresponds to a ECMA-335 MethodDef record. It is however not a 1:1
/// mapping because a method could be used in the AOT compiled program without generating
/// the reflection metadata for it (which would not be possible in IL terms).
/// </summary>
/// <remarks>
/// Only expected to be used during ILScanning when scanning for reflection.
Expand Down
Expand Up @@ -41,7 +41,7 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
&& ecmaModule.EntryPoint is MethodDesc entrypoint
&& !factory.MetadataManager.IsReflectionBlocked(entrypoint))
{
dependencies.Add(factory.ReflectableMethod(entrypoint), "Reflectable entrypoint");
dependencies.Add(factory.ReflectedMethod(entrypoint), "Reflectable entrypoint");
}

CustomAttributeBasedDependencyAlgorithm.AddDependenciesDueToCustomAttributes(ref dependencies, factory, (EcmaAssembly)_module);
Expand Down
Expand Up @@ -282,14 +282,19 @@ private void CreateNodeCaches()
return new TypeGVMEntriesNode(type);
});

_reflectableMethods = new NodeCache<MethodDesc, ReflectableMethodNode>(method =>
_reflectedMethods = new NodeCache<MethodDesc, ReflectedMethodNode>(method =>
{
return new ReflectableMethodNode(method);
return new ReflectedMethodNode(method);
});

_reflectableFields = new NodeCache<FieldDesc, ReflectableFieldNode>(field =>
_reflectedFields = new NodeCache<FieldDesc, ReflectedFieldNode>(field =>
{
return new ReflectableFieldNode(field);
return new ReflectedFieldNode(field);
});

_reflectedTypes = new NodeCache<TypeDesc, ReflectedTypeNode>(type =>
{
return new ReflectedTypeNode(type);
});

_genericStaticBaseInfos = new NodeCache<MetadataType, GenericStaticBaseInfoNode>(type =>
Expand Down Expand Up @@ -899,16 +904,22 @@ internal TypeGVMEntriesNode TypeGVMEntries(TypeDesc type)
return _gvmTableEntries.GetOrAdd(type);
}

private NodeCache<MethodDesc, ReflectableMethodNode> _reflectableMethods;
public ReflectableMethodNode ReflectableMethod(MethodDesc method)
private NodeCache<MethodDesc, ReflectedMethodNode> _reflectedMethods;
public ReflectedMethodNode ReflectedMethod(MethodDesc method)
{
return _reflectedMethods.GetOrAdd(method);
}

private NodeCache<FieldDesc, ReflectedFieldNode> _reflectedFields;
public ReflectedFieldNode ReflectedField(FieldDesc field)
{
return _reflectableMethods.GetOrAdd(method);
return _reflectedFields.GetOrAdd(field);
}

private NodeCache<FieldDesc, ReflectableFieldNode> _reflectableFields;
public ReflectableFieldNode ReflectableField(FieldDesc field)
private NodeCache<TypeDesc, ReflectedTypeNode> _reflectedTypes;
public ReflectedTypeNode ReflectedType(TypeDesc type)
{
return _reflectableFields.GetOrAdd(field);
return _reflectedTypes.GetOrAdd(type);
}

private NodeCache<MetadataType, GenericStaticBaseInfoNode> _genericStaticBaseInfos;
Expand Down
Expand Up @@ -13,12 +13,15 @@ namespace ILCompiler.DependencyAnalysis
{
/// <summary>
/// Represents a field that is gettable/settable from reflection.
/// The field can be on a non-generic type, generic type definition, or an instantiatied type.
/// To match IL semantics, we maintain that a field on a generic type will be consistently
/// reflection-accessible. Either the field is accessible on all instantiations or on none of them.
/// </summary>
public class ReflectableFieldNode : DependencyNodeCore<NodeFactory>
public class ReflectedFieldNode : DependencyNodeCore<NodeFactory>
{
private readonly FieldDesc _field;

public ReflectableFieldNode(FieldDesc field)
public ReflectedFieldNode(FieldDesc field)
{
Debug.Assert(!field.OwningType.IsCanonicalSubtype(CanonicalFormKind.Any)
|| field.OwningType.ConvertToCanonForm(CanonicalFormKind.Specific) == field.OwningType);
Expand Down Expand Up @@ -46,7 +49,7 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
// Ensure we consistently apply reflectability to all fields sharing the same definition.
// Bases for different instantiations of the field have a conditional dependency on the definition node that
// brings a ReflectableField of the instantiated field if it's necessary for it to be reflectable.
dependencies.Add(factory.ReflectableField(typicalField), "Definition of the reflectable field");
dependencies.Add(factory.ReflectedField(typicalField), "Definition of the reflectable field");
}

// Runtime reflection stack needs to see the type handle of the owning type
Expand Down
Expand Up @@ -13,12 +13,16 @@ namespace ILCompiler.DependencyAnalysis
{
/// <summary>
/// Represents a method that is visible to reflection.
/// The method can be on a non-generic type, generic type definition, or an instantiatied type.
/// To match IL semantics, we maintain that a method on a generic type will be consistently
/// reflection-accessible. Either the method is accessible on all instantiations or on none of them.
/// Similar invariants hold for generic methods.
/// </summary>
public class ReflectableMethodNode : DependencyNodeCore<NodeFactory>
public class ReflectedMethodNode : DependencyNodeCore<NodeFactory>
{
private readonly MethodDesc _method;

public ReflectableMethodNode(MethodDesc method)
public ReflectedMethodNode(MethodDesc method)
{
Debug.Assert(!method.IsCanonicalMethod(CanonicalFormKind.Any) ||
method.GetCanonMethodTarget(CanonicalFormKind.Specific) == method);
Expand Down Expand Up @@ -46,13 +50,13 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
MethodDesc typicalMethod = _method.GetTypicalMethodDefinition();
if (typicalMethod != _method)
{
dependencies.Add(factory.ReflectableMethod(typicalMethod), "Definition of the reflectable method");
dependencies.Add(factory.ReflectedMethod(typicalMethod), "Definition of the reflectable method");
}

MethodDesc canonMethod = _method.GetCanonMethodTarget(CanonicalFormKind.Specific);
if (canonMethod != _method)
{
dependencies.Add(factory.ReflectableMethod(canonMethod), "Canonical version of the reflectable method");
dependencies.Add(factory.ReflectedMethod(canonMethod), "Canonical version of the reflectable method");
}

// Make sure we generate the method body and other artifacts.
Expand Down
@@ -0,0 +1,54 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;

using ILCompiler.DependencyAnalysisFramework;

using Internal.TypeSystem;

using Debug = System.Diagnostics.Debug;

namespace ILCompiler.DependencyAnalysis
{
/// <summary>
/// Represents a type that is forced to be visible from reflection.
/// The system needs to implicitly assume that any allocated type could be visible from
/// reflection due to <see cref="object.GetType" />, so presence of this node is not
/// a necessary condition for a type to be reflection visible. However, the presence of this
/// node indicates that a new reflectable type was forced into existence by e.g. dataflow
/// analysis, and is not just a byproduct of allocating an instance of this type.
/// </summary>
public class ReflectedTypeNode : DependencyNodeCore<NodeFactory>
{
private readonly TypeDesc _type;

public ReflectedTypeNode(TypeDesc type)
{
Debug.Assert(!type.IsCanonicalSubtype(CanonicalFormKind.Any)
|| type.ConvertToCanonForm(CanonicalFormKind.Specific) == type);
_type = type;
}

public TypeDesc Type => _type;

public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory factory)
{
return new DependencyListEntry[]
{
new DependencyListEntry(factory.MaximallyConstructableType(_type), "Reflection target"),
};
}
protected override string GetName(NodeFactory factory)
{
return "Reflectable type: " + _type.ToString();
}

public override bool InterestingForDynamicDependencyAnalysis => false;
public override bool HasDynamicDependencies => false;
public override bool HasConditionalStaticDependencies => false;
public override bool StaticDependenciesAreComputed => true;
public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependencies(NodeFactory factory) => null;
public override IEnumerable<CombinedDependencyListEntry> SearchDynamicDependencies(List<DependencyNodeCore<NodeFactory>> markedNodes, int firstNode, NodeFactory factory) => null;
}
}
Expand Up @@ -14,6 +14,9 @@ namespace ILCompiler.DependencyAnalysis
{
/// <summary>
/// Represents a type that has metadata generated in the current compilation.
/// This node corresponds to an ECMA-335 TypeDef record. It is however not a 1:1
/// mapping because IL could be compiled into machine code without generating a record
/// in the reflection metadata (which would not be possible in IL terms).
/// </summary>
/// <remarks>
/// Only expected to be used during ILScanning when scanning for reflection.
Expand Down Expand Up @@ -48,7 +51,7 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
{
// A lot of the enum reflection actually happens on top of the respective MethodTable (e.g. getting the underlying type),
// so for enums also include their MethodTable.
dependencies.Add(factory.MaximallyConstructableType(_type), "Reflectable enum");
dependencies.Add(factory.ReflectedType(_type), "Reflectable enum");

// Enums are not useful without their literal fields. The literal fields are not referenced
// from anywhere (source code reference to enums compiles to the underlying numerical constants in IL).
Expand Down
Expand Up @@ -213,7 +213,7 @@ protected override void ProcessField(TypeDesc type, FieldDesc field, XPathNaviga
// LogWarning(nav, DiagnosticId.XmlDuplicatePreserveMember, field.FullName);
}*/

_dependencies.Add(_factory.ReflectableField(field), "field kept due to descriptor");
_dependencies.Add(_factory.ReflectedField(field), "field kept due to descriptor");
}

protected override void ProcessMethod(TypeDesc type, MethodDesc method, XPathNavigator nav, object? customData)
Expand All @@ -228,11 +228,11 @@ protected override void ProcessMethod(TypeDesc type, MethodDesc method, XPathNav
if (customData is bool required && !required)
{
//TODO: Add a conditional dependency if the type is used also mark the method
_dependencies.Add(_factory.ReflectableMethod(method), "method kept due to descriptor");
_dependencies.Add(_factory.ReflectedMethod(method), "method kept due to descriptor");
}
else
{
_dependencies.Add(_factory.ReflectableMethod(method), "method kept due to descriptor");
_dependencies.Add(_factory.ReflectedMethod(method), "method kept due to descriptor");
}
}

Expand Down

0 comments on commit edadf5f

Please sign in to comment.