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

Improve the performance of the type loader through various tweaks #85743

Merged
merged 29 commits into from
Jun 13, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ff0e821
Skip type validation by default in ReadyToRun images
davidwrighton Apr 13, 2023
3b8143c
Replace expensive lookups of generic parameter and nested class data …
davidwrighton Apr 20, 2023
0c88982
Store index of MethodDesc as well as ChunkIndex. Makes MethodDesc::Ge…
davidwrighton Apr 21, 2023
910f4c4
Optimize the path for computing if a method is eligible for tiered co…
davidwrighton Apr 22, 2023
418af3c
Remove CanShareVtableChunksFrom concept
davidwrighton May 3, 2023
c1c9983
Merge branch 'main' of github.com:dotnet/runtime into faster_type_load
davidwrighton May 8, 2023
48dfa0e
Fix up some more issues
davidwrighton May 11, 2023
d274c51
Bring back late virtual propagation in the presence of covariant over…
davidwrighton May 11, 2023
5bae879
Check correct flag on EEClass
davidwrighton May 12, 2023
9f9e586
Drive by fix for GetRestoredSlot. We don't need the handling of unres…
davidwrighton May 12, 2023
f5e72a6
Fix composite build with new tables
davidwrighton May 12, 2023
8eaf4a0
Uniquify the mangled names
davidwrighton May 15, 2023
ac7ec65
Add more __
davidwrighton May 17, 2023
1a57a0a
Initial pass at type skip verifation checker
davidwrighton May 19, 2023
5f770c6
Fix logging and some correctness issues
davidwrighton May 19, 2023
52bd220
Enable the more of type checking
davidwrighton May 20, 2023
dfecc6f
Fix build breaks involving new feature of GenericParameterDesc
davidwrighton May 30, 2023
6ef05ec
Merge branch 'main' of github.com:dotnet/runtime into faster_type_load
davidwrighton May 30, 2023
eff81d5
Add documentation for R2R format changes
davidwrighton Jun 1, 2023
db08221
Fix implementation of CompareMethodContraints. instead of using IsGen…
davidwrighton Jun 1, 2023
3b21879
Fix nits noticed by Aaron
davidwrighton Jun 1, 2023
06b6ffd
Add some const correctness to the world
davidwrighton Jun 1, 2023
df3a418
Fix issues noted by Michal, as well as remaining constrain checking i…
davidwrighton Jun 2, 2023
75144c4
Merge branch 'main' of github.com:dotnet/runtime into faster_type_load
davidwrighton Jun 5, 2023
1f44575
Code review details
davidwrighton Jun 5, 2023
e6c0b4b
Merge branch 'main' of github.com:dotnet/runtime into faster_type_load
davidwrighton Jun 9, 2023
0f45839
Merge branch 'main' into faster_type_load
davidwrighton Jun 9, 2023
2648965
Code review from trylek
davidwrighton Jun 12, 2023
bc127bb
Merge branch 'main' of https://github.com/dotnet/runtime into faster_…
davidwrighton Jun 13, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/coreclr/inc/readytorun.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ enum class ReadyToRunSectionType : uint32_t
ManifestAssemblyMvids = 118, // Added in V5.3
CrossModuleInlineInfo = 119, // Added in V6.2
HotColdMap = 120, // Added in V8.0
MethodIsGenericMap = 121, // Added in V9.0
EnclosingTypeMap = 122, // Added in V9.0
TypeGenericInfoMap = 123, // Added in V9.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MethodIsGenericMap = 121, // Added in V9.0
EnclosingTypeMap = 122, // Added in V9.0
TypeGenericInfoMap = 123, // Added in V9.0
MethodIsGenericMap = 121, // Added in V9.0
EnclosingTypeMap = 122, // Added in V9.0
TypeGenericInfoMap = 123, // Added in V9.0


// If you add a new section consider whether it is a breaking or non-breaking change.
// Usually it is non-breaking, but if it is preferable to have older runtimes fail
Expand Down Expand Up @@ -121,6 +124,21 @@ enum class ReadyToRunImportSectionFlags : uint16_t
PCode = 0x0004, // Section contains pointers to code
};

enum class ReadyToRunTypeGenericInfo : uint8_t
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
GenericCountMask = 0x3,
HasConstraints = 0x4,
HasVariance = 0x8,
};

enum class ReadyToRunGenericInfoGenericCount : uint32_t
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
Zero = 0,
One = 1,
Two = 2,
MoreThanTwo = 3
};

//
// READYTORUN_IMPORT_SECTION describes image range with references to code or runtime data structures
//
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ public enum ReadyToRunSectionType
ManifestAssemblyMvids = 118, // Added in 5.3
CrossModuleInlineInfo = 119, // Added in 6.3
HotColdMap = 120, // Added in 8.0
MethodIsGenericMap = 121, // Added in V9.0
EnclosingTypeMap = 122, // Added in V9.0
TypeGenericInfoMap = 123, // Added in V9.0

//
// NativeAOT ReadyToRun sections
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ namespace Internal.ReadyToRunConstants
public enum ReadyToRunFlags
{
READYTORUN_FLAG_PlatformNeutralSource = 0x00000001, // Set if the original IL assembly was platform-neutral
READYTORUN_FLAG_SkipTypeValidation = 0x00000002, // Set of methods with native code was determined using profile data
READYTORUN_FLAG_Partial = 0x00000004,
READYTORUN_FLAG_SkipTypeValidation = 0x00000002, // Runtime should trust that the metadata for the types defined in this module is correct
READYTORUN_FLAG_Partial = 0x00000004, // Set of methods with native code was determined using profile data
READYTORUN_FLAG_NonSharedPInvokeStubs = 0x00000008, // PInvoke stubs compiled into image are non-shareable (no secret parameter)
READYTORUN_FLAG_EmbeddedMSIL = 0x00000010, // MSIL is embedded in the composite R2R executable
READYTORUN_FLAG_Component = 0x00000020, // This is the header describing a component assembly of composite R2R
Expand Down Expand Up @@ -89,6 +89,23 @@ internal enum ReadyToRunCrossModuleInlineFlags : uint
InlinerRidShift = 1,
};

[Flags]
public enum ReadyToRunTypeGenericInfo : byte
{
GenericCountMask = 0x3,
HasConstraints = 0x4,
HasVariance = 0x8,
}

public enum ReadyToRunGenericInfoGenericCount : uint
{
Zero = 0,
One = 1,
Two = 2,
MoreThanTwo = 3
}

AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved

public enum DictionaryEntryKind
{
EmptySlot = 0,
Expand Down
12 changes: 11 additions & 1 deletion src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,19 @@ private static bool CanCastGenericParameterTo(this GenericParameterDesc thisType
return true;
}

InstantiationContext context;
if (thisType.AssociatedTypeOrMethod is MethodDesc method)
{
context = new InstantiationContext(method.OwningType.Instantiation, method.Instantiation);
}
else
{
context = new InstantiationContext(((TypeDesc)thisType.AssociatedTypeOrMethod).Instantiation, default(Instantiation));
}
foreach (var typeConstraint in thisType.TypeConstraints)
{
if (typeConstraint.CanCastToInternal(otherType, protect))
TypeDesc instantiatedConstraint = typeConstraint.InstantiateSignature(context.TypeInstantiation, context.MethodInstantiation);
if (instantiatedConstraint.CanCastToInternal(otherType, protect))
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ public virtual string Name
/// </summary>
public abstract int Index { get; }

/// <summary>
/// The associated type or method which defines this Generic Parameter
/// </summary>
public abstract TypeSystemEntity AssociatedTypeOrMethod { get; }


AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
/// <summary>
/// Gets a value indicating the variance of this generic parameter.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ protected override TypeFlags ComputeTypeFlags(TypeFlags mask)
flags |= TypeFlags.SignatureTypeVariable;
}

flags |= TypeFlags.HasGenericVarianceComputed;
flags |= TypeFlags.HasFinalizerComputed;
flags |= TypeFlags.AttributeCacheComputed;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a bug if someone asks for these? Signature variables don't have "a finalizer" but also if someone is asking for that, maybe it's a bug. I've ran into the assert not setting this flag produces a 100% of times this was a bug in my code that I was happy to find so easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, this work operates almost entirely over uninstantiated types and methods, and has a few spots where it actually needs to use these things. However, it might be the case that I can only set a minimal number of flags. I enabled them all as it seemed like the right thing to do, but I believe the only 1 I really needed to work was the attribute cache.

Copy link
Member

Choose a reason for hiding this comment

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

What's the stack that leads to this being needed? This really feels like something where the only right answer is to unask the question.


return flags;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

using System.Collections.Generic;
using System.Reflection.Metadata;

using System.Threading;
using Debug = System.Diagnostics.Debug;
using GenericParameterAttributes = System.Reflection.GenericParameterAttributes;

Expand All @@ -13,6 +13,7 @@ public sealed partial class EcmaGenericParameter : GenericParameterDesc
{
private EcmaModule _module;
private GenericParameterHandle _handle;
private TypeSystemEntity _associatedTypeOrMethod;

internal EcmaGenericParameter(EcmaModule module, GenericParameterHandle handle)
{
Expand Down Expand Up @@ -78,6 +79,22 @@ public override GenericParameterKind Kind
}
}

public override TypeSystemEntity AssociatedTypeOrMethod
{
get
{
TypeSystemEntity tse = Volatile.Read(ref _associatedTypeOrMethod);
if (tse == null)
{
GenericParameter parameter = _module.MetadataReader.GetGenericParameter(_handle);

tse = (TypeSystemEntity)_module.GetObject(parameter.Parent);
Volatile.Write(ref _associatedTypeOrMethod, tse);
}
return tse;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the Volatile here.

If this is perf critical, we can use the same pattern as BaseType:

public override DefType BaseType
{
get
{
if (_baseType == this)
return InitializeBaseType();
return _baseType;
}
}

If it's not perf critical maybe we don't even need to cache it.

}
}

public override int Index
{
get
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;

using Internal.Text;
using Internal.TypeSystem.Ecma;
using System.Diagnostics;
using System.Reflection;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using Internal.ReadyToRunConstants;

namespace ILCompiler.DependencyAnalysis.ReadyToRun
{
public class EnclosingTypeMapNode : ModuleSpecificHeaderTableNode
{
private MetadataReader _metadata;

public EnclosingTypeMapNode(EcmaModule module) : base(module)
{
_metadata = module.MetadataReader;
// This map is only valid for assemblies with <= 0xFFFE types defined within
if (!IsSupported(_metadata))
throw new InternalCompilerErrorException("EnclosingTypeMap made for assembly with more than 0xFFFE types");
}

public static bool IsSupported(MetadataReader metadata)
{
// This map is only valid for assemblies with <= 0xFFFE types defined within
// and really shouldn't be generated for tiny assemblies, as the map provides very little to no value
// in those situations
int typeDefinitionCount = metadata.TypeDefinitions.Count;

return ((typeDefinitionCount > 10) && (typeDefinitionCount <= 0xFFFE));
}

public override int ClassCode => 990540812;
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved

protected override string ModuleSpecificName => "__EnclosingTypeMap__";

public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
{
// This node does not trigger generation of other nodes.
if (relocsOnly)
return new ObjectData(Array.Empty<byte>(), Array.Empty<Relocation>(), 1, new ISymbolDefinitionNode[] { this });

ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly);
builder.AddSymbol(this);

// This map is only valid for assemblies with <= 0xFFFE types defined within
davidwrighton marked this conversation as resolved.
Show resolved Hide resolved
builder.EmitUShort(checked((ushort)_metadata.TypeDefinitions.Count));

foreach (var typeDefinitionHandle in _metadata.TypeDefinitions)
{
var typeDefinition = _metadata.GetTypeDefinition(typeDefinitionHandle);
if (!typeDefinition.IsNested)
{
builder.EmitUShort(0);
}
else
{
builder.EmitUShort(checked((ushort)MetadataTokens.GetRowNumber(typeDefinition.GetDeclaringType())));
}
}

return builder.ToObjectData();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
using Internal.Runtime;
using Internal.Text;
using Internal.TypeSystem;
using Internal.TypeSystem.Ecma;
using Internal.ReadyToRunConstants;
using ILCompiler.DependencyAnalysisFramework;
using System.Threading.Tasks;

namespace ILCompiler.DependencyAnalysis.ReadyToRun
{
Expand All @@ -33,6 +35,50 @@ public override ObjectNodeSection GetSection(NodeFactory factory)
}
}

public abstract class ModuleSpecificHeaderTableNode : HeaderTableNode
davidwrighton marked this conversation as resolved.
Show resolved Hide resolved
{
protected readonly EcmaModule _module;

public ModuleSpecificHeaderTableNode(EcmaModule module)
{
_module = module;
}

public sealed override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
{
ModuleSpecificHeaderTableNode otherModuleSpecificHeaderTableNode = (ModuleSpecificHeaderTableNode)other;

if (_module == null)
{
Debug.Assert(otherModuleSpecificHeaderTableNode._module != null);
return -1;
}
else if (otherModuleSpecificHeaderTableNode._module == null)
{
return 1;
}

return _module.Assembly.GetName().Name.CompareTo(otherModuleSpecificHeaderTableNode._module.Assembly.GetName().Name);
}

protected abstract string ModuleSpecificName { get; }

public sealed override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb)
{
sb.Append(nameMangler.CompilationUnitPrefix);
if (_module != null)
{
sb.Append(ModuleSpecificName);
sb.Append(_module.Assembly.GetName().Name);
}
else
{
sb.Append(ModuleSpecificName);
}

}
}

public abstract class HeaderNode : ObjectNode, ISymbolDefinitionNode
{
struct HeaderItem
Expand All @@ -51,9 +97,19 @@ public HeaderItem(ReadyToRunSectionType id, DependencyNodeCore<NodeFactory> node

private readonly List<HeaderItem> _items = new List<HeaderItem>();
private readonly ReadyToRunFlags _flags;
private readonly Task<(bool canSkipValidation, string[] reasons)> _shouldAddSkipTypeValidationFlag;

public HeaderNode(ReadyToRunFlags flags)
public HeaderNode(ReadyToRunFlags flags, EcmaModule moduleToCheckForSkipTypeValidation)
{

if (moduleToCheckForSkipTypeValidation != null)
{
_shouldAddSkipTypeValidationFlag = TypeValidationChecker.CanSkipValidation(moduleToCheckForSkipTypeValidation);
}
else
{
_shouldAddSkipTypeValidationFlag = Task.FromResult((false, new string[0]));
}
_flags = flags;
}

Expand Down Expand Up @@ -94,7 +150,19 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
_items.MergeSort((x, y) => Comparer<int>.Default.Compare((int)x.Id, (int)y.Id));

// ReadyToRunHeader.Flags
builder.EmitInt((int)_flags);
int flagsInt = (int)_flags;
if (_shouldAddSkipTypeValidationFlag.Result.canSkipValidation)
{
flagsInt |= (int)ReadyToRunFlags.READYTORUN_FLAG_SkipTypeValidation;
}
else
{
//#if DIAGNOSE_TYPE_VALIDATION_RULES
// foreach (string reason in _shouldAddSkipTypeValidationFlag.Result.reasons)
// System.Console.WriteLine(reason);
//#endif
}
builder.EmitInt(flagsInt);

// ReadyToRunHeader.NumberOfSections
ObjectDataBuilder.Reservation sectionCountReservation = builder.ReserveInt();
Expand Down Expand Up @@ -137,8 +205,8 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)

public class GlobalHeaderNode : HeaderNode
{
public GlobalHeaderNode(ReadyToRunFlags flags)
: base(flags)
public GlobalHeaderNode(ReadyToRunFlags flags, EcmaModule moduleToCheckForSkipValidation)
: base(flags, moduleToCheckForSkipValidation)
{
}

Expand Down Expand Up @@ -166,7 +234,7 @@ public class AssemblyHeaderNode : HeaderNode
private readonly int _index;

public AssemblyHeaderNode(ReadyToRunFlags flags, int index)
: base(flags)
: base(flags, null)
{
_index = index;
}
Expand Down