-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changes from 22 commits
ff0e821
3b8143c
0c88982
910f4c4
418af3c
c1c9983
48dfa0e
d274c51
5bae879
9f9e586
f5e72a6
8eaf4a0
ac7ec65
1a57a0a
5f770c6
52bd220
dfecc6f
6ef05ec
eff81d5
db08221
3b21879
06b6ffd
df3a418
75144c4
1f44575
e6c0b4b
0f45839
2648965
bc127bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ internal class RuntimeGenericParameterDesc : GenericParameterDesc | |
private readonly int _index; | ||
private readonly TypeSystemContext _context; | ||
private readonly GenericVariance _variance; | ||
private TypeSystemEntity _associatedTypeOrMethod; | ||
|
||
public RuntimeGenericParameterDesc(GenericParameterKind kind, int index, TypeSystemContext context, GenericVariance variance) | ||
{ | ||
|
@@ -25,5 +26,12 @@ public RuntimeGenericParameterDesc(GenericParameterKind kind, int index, TypeSys | |
public override TypeSystemContext Context => _context; | ||
|
||
public override GenericVariance Variance => _variance; | ||
|
||
public override TypeSystemEntity AssociatedTypeOrMethod => _associatedTypeOrMethod; | ||
|
||
internal void SetAssociatedTypeOrMethod(TypeSystemEntity associatedTypeOrMethod) | ||
{ | ||
_associatedTypeOrMethod = associatedTypeOrMethod; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do this in a constructor? This is already a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor requested and implemented. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,10 @@ protected override TypeFlags ComputeTypeFlags(TypeFlags mask) | |
flags |= TypeFlags.SignatureTypeVariable; | ||
} | ||
|
||
flags |= TypeFlags.HasGenericVarianceComputed; | ||
flags |= TypeFlags.HasFinalizerComputed; | ||
flags |= TypeFlags.AttributeCacheComputed; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -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) | ||||||||||||||||||||
{ | ||||||||||||||||||||
|
@@ -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; | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: runtime/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaType.cs Lines 174 to 182 in 8ee61fe
If it's not perf critical maybe we don't even need to cache it. |
||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
public override int Index | ||||||||||||||||||||
{ | ||||||||||||||||||||
get | ||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,8 @@ private InstantiatedGenericParameter(GenericParameterDesc genericParam) | |
|
||
public override GenericConstraints Constraints => _genericParam.Constraints; | ||
|
||
public override TypeSystemEntity AssociatedTypeOrMethod => _genericParam.AssociatedTypeOrMethod; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know much about the ILVerify codebase, but doesn't this need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe? I don't know that codebase either, but it doesn't seem unreasonable. |
||
|
||
public override IEnumerable<TypeDesc> TypeConstraints | ||
{ | ||
get | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// 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
|
||
Debug.Assert(_metadata.TypeDefinitions.Count <= 0xFFFE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit - this constant appears multiple times in the code, could we make it a symbolic constant? A sufficiently expressive name like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. |
||
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(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.