From 2e40a84445499e9887d8ebc869871093a14e92b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 4 Apr 2023 16:34:40 +0900 Subject: [PATCH 1/5] Speed up named type lookups When the type system needs to resolve a named type in a module, it will do a `foreach` loop over all types in the module looking for the type. This can get mildly hot and I've seen it in CPU profiles but it never looked too important to address (despite the TODO). But when MIBC files are passed to the compiler, this gets ridiculously hot. Compile Hello world by default: 0.98 seconds. Compile hello world with 5 MIBC files: 9.1 seconds. This adds a hashtable to the lookup and drops the MIBC case to 1.4 seconds (we'll want to parallelize the MIBC loading on a background thread to get rid of the last mile, but first things first). --- .../Common/TypeSystem/Ecma/EcmaModule.cs | 114 +++++++++++++++--- 1 file changed, 99 insertions(+), 15 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs index 1c75279af10adb..464bd7bd42362c 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs @@ -7,6 +7,7 @@ using System.Reflection.Metadata; using System.Reflection.Metadata.Ecma335; using System.Reflection.PortableExecutable; +using System.Text; using Debug = System.Diagnostics.Debug; @@ -14,8 +15,8 @@ namespace Internal.TypeSystem.Ecma { public partial class EcmaModule : ModuleDesc { - private PEReader _peReader; - protected MetadataReader _metadataReader; + private readonly PEReader _peReader; + protected readonly MetadataReader _metadataReader; internal interface IEntityHandleObject { @@ -282,27 +283,110 @@ public bool IsPlatformNeutral } } + private TypeDefinitionHandle[] _typeDefinitionBuckets; + private volatile TypeDefinitionHandle[] _typeDefinitionBucketHeads; + + private TypeDefinitionHandle[] InitializeFindNamedType() + { + TypeDefinitionHandle[] buckets = new TypeDefinitionHandle[_metadataReader.TypeDefinitions.Count + 1]; + TypeDefinitionHandle[] bucketHeads = new TypeDefinitionHandle[(buckets.Length / 8) + 1]; + + MetadataReader reader = _metadataReader; + foreach (TypeDefinitionHandle typeHandle in reader.TypeDefinitions) + { + TypeDefinition typeDef = reader.GetTypeDefinition(typeHandle); + if (typeDef.Attributes.IsNested()) + continue; + + var hashCode = default(HashCode); + AppendHashCode(ref hashCode, reader.GetBlobReader(typeDef.Namespace)); + AppendHashCode(ref hashCode, reader.GetBlobReader(typeDef.Name)); + + ref TypeDefinitionHandle head = ref bucketHeads[(uint)hashCode.ToHashCode() % bucketHeads.Length]; + ref TypeDefinitionHandle entry = ref buckets[MetadataTokens.GetRowNumber(typeHandle)]; + + entry = head; + head = typeHandle; + } + + _typeDefinitionBuckets = buckets; + _typeDefinitionBucketHeads = bucketHeads; + + return bucketHeads; + + static unsafe void AppendHashCode(ref HashCode hashCode, BlobReader reader) + { + while (reader.RemainingBytes > 0) + hashCode.Add(reader.ReadByte()); + } + } + + private TypeDefinitionHandle FindType(string nameSpace, string name) + { + var hashCodeBuilder = default(HashCode); + AppendHashCode(ref hashCodeBuilder, nameSpace); + AppendHashCode(ref hashCodeBuilder, name); + + MetadataReader reader = _metadataReader; + MetadataStringComparer stringComparer = reader.StringComparer; + + TypeDefinitionHandle[] bucketHeads = _typeDefinitionBucketHeads ?? InitializeFindNamedType(); + TypeDefinitionHandle entry = bucketHeads[(uint)hashCodeBuilder.ToHashCode() % bucketHeads.Length]; + while (!entry.IsNil) + { + var typeDefinition = reader.GetTypeDefinition(entry); + if (stringComparer.Equals(typeDefinition.Name, name) && + stringComparer.Equals(typeDefinition.Namespace, nameSpace)) + { + return entry; + } + + entry = _typeDefinitionBuckets[MetadataTokens.GetRowNumber(entry)]; + } + + return default; + + static void AppendHashCodeNonAscii(ref HashCode hashCode, ReadOnlySpan s) + { + const int MaxStackAlloc = 1024; + int maxByteCount = Encoding.UTF8.GetMaxByteCount(s.Length); + Span utf8 = maxByteCount <= MaxStackAlloc ? stackalloc byte[MaxStackAlloc] : new byte[maxByteCount]; + int actualBytes = Encoding.UTF8.GetBytes(s, utf8); + + foreach (byte b in utf8.Slice(0, actualBytes)) + hashCode.Add(b); + } + + static void AppendHashCode(ref HashCode hashCode, string s) + { + for (int i = 0; i < s.Length; i++) + { + if (s[i] <= 0x7F) + { + hashCode.Add((byte)s[i]); + } + else + { + AppendHashCodeNonAscii(ref hashCode, ((ReadOnlySpan)s).Slice(i)); + return; + } + } + } + } + + public sealed override object GetType(string nameSpace, string name, NotFoundBehavior notFoundBehavior) { var currentModule = this; // src/coreclr/vm/clsload.cpp use the same restriction to detect a loop in the type forwarding. for (int typeForwardingChainSize = 0; typeForwardingChainSize <= 1024; typeForwardingChainSize++) { + TypeDefinitionHandle foundType = currentModule.FindType(nameSpace, name); + if (!foundType.IsNil) + return currentModule.GetType(foundType); + var metadataReader = currentModule._metadataReader; var stringComparer = metadataReader.StringComparer; - // TODO: More efficient implementation? - foreach (var typeDefinitionHandle in metadataReader.TypeDefinitions) - { - var typeDefinition = metadataReader.GetTypeDefinition(typeDefinitionHandle); - if (typeDefinition.Attributes.IsNested()) - continue; - - if (stringComparer.Equals(typeDefinition.Name, name) && - stringComparer.Equals(typeDefinition.Namespace, nameSpace)) - { - return currentModule.GetType(typeDefinitionHandle); - } - } foreach (var exportedTypeHandle in metadataReader.ExportedTypes) { From a49ec78bfd39a5626fe5aa566278707ddd68b901 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 5 Apr 2023 10:06:19 +0900 Subject: [PATCH 2/5] =?UTF-8?q?Na=C3=AFve=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Common/TypeSystem/Ecma/EcmaModule.cs | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs index 1c75279af10adb..9a39af053bbea1 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs @@ -282,28 +282,36 @@ public bool IsPlatformNeutral } } + private volatile Dictionary<(string Name, string Namespace), TypeDefinitionHandle> _lookup; + + private Dictionary<(string Name, string Namespace), TypeDefinitionHandle> CreateLookup() + { + var result = new Dictionary<(string Name, string Namespace), TypeDefinitionHandle>(); + + var metadataReader = _metadataReader; + foreach (var typeDefinitionHandle in metadataReader.TypeDefinitions) + { + var typeDefinition = metadataReader.GetTypeDefinition(typeDefinitionHandle); + if (typeDefinition.Attributes.IsNested()) + continue; + + result.Add((metadataReader.GetString(typeDefinition.Name), metadataReader.GetString(typeDefinition.Namespace)), typeDefinitionHandle); + } + + return _lookup = result; + } + public sealed override object GetType(string nameSpace, string name, NotFoundBehavior notFoundBehavior) { var currentModule = this; // src/coreclr/vm/clsload.cpp use the same restriction to detect a loop in the type forwarding. for (int typeForwardingChainSize = 0; typeForwardingChainSize <= 1024; typeForwardingChainSize++) { + if ((currentModule._lookup ?? currentModule.CreateLookup()).TryGetValue((name, nameSpace), out TypeDefinitionHandle typeDefHandle)) + return currentModule.GetType(typeDefHandle); + var metadataReader = currentModule._metadataReader; var stringComparer = metadataReader.StringComparer; - // TODO: More efficient implementation? - foreach (var typeDefinitionHandle in metadataReader.TypeDefinitions) - { - var typeDefinition = metadataReader.GetTypeDefinition(typeDefinitionHandle); - if (typeDefinition.Attributes.IsNested()) - continue; - - if (stringComparer.Equals(typeDefinition.Name, name) && - stringComparer.Equals(typeDefinition.Namespace, nameSpace)) - { - return currentModule.GetType(typeDefinitionHandle); - } - } - foreach (var exportedTypeHandle in metadataReader.ExportedTypes) { var exportedType = metadataReader.GetExportedType(exportedTypeHandle); From a5d978817da263c2706465a52f776e4fa3d11d83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 5 Apr 2023 13:26:30 +0900 Subject: [PATCH 3/5] Revert "Speed up named type lookups" This reverts commit 2e40a84445499e9887d8ebc869871093a14e92b7. --- .../Common/TypeSystem/Ecma/EcmaModule.cs | 114 +++--------------- 1 file changed, 15 insertions(+), 99 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs index 464bd7bd42362c..1c75279af10adb 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs @@ -7,7 +7,6 @@ using System.Reflection.Metadata; using System.Reflection.Metadata.Ecma335; using System.Reflection.PortableExecutable; -using System.Text; using Debug = System.Diagnostics.Debug; @@ -15,8 +14,8 @@ namespace Internal.TypeSystem.Ecma { public partial class EcmaModule : ModuleDesc { - private readonly PEReader _peReader; - protected readonly MetadataReader _metadataReader; + private PEReader _peReader; + protected MetadataReader _metadataReader; internal interface IEntityHandleObject { @@ -283,110 +282,27 @@ public bool IsPlatformNeutral } } - private TypeDefinitionHandle[] _typeDefinitionBuckets; - private volatile TypeDefinitionHandle[] _typeDefinitionBucketHeads; - - private TypeDefinitionHandle[] InitializeFindNamedType() - { - TypeDefinitionHandle[] buckets = new TypeDefinitionHandle[_metadataReader.TypeDefinitions.Count + 1]; - TypeDefinitionHandle[] bucketHeads = new TypeDefinitionHandle[(buckets.Length / 8) + 1]; - - MetadataReader reader = _metadataReader; - foreach (TypeDefinitionHandle typeHandle in reader.TypeDefinitions) - { - TypeDefinition typeDef = reader.GetTypeDefinition(typeHandle); - if (typeDef.Attributes.IsNested()) - continue; - - var hashCode = default(HashCode); - AppendHashCode(ref hashCode, reader.GetBlobReader(typeDef.Namespace)); - AppendHashCode(ref hashCode, reader.GetBlobReader(typeDef.Name)); - - ref TypeDefinitionHandle head = ref bucketHeads[(uint)hashCode.ToHashCode() % bucketHeads.Length]; - ref TypeDefinitionHandle entry = ref buckets[MetadataTokens.GetRowNumber(typeHandle)]; - - entry = head; - head = typeHandle; - } - - _typeDefinitionBuckets = buckets; - _typeDefinitionBucketHeads = bucketHeads; - - return bucketHeads; - - static unsafe void AppendHashCode(ref HashCode hashCode, BlobReader reader) - { - while (reader.RemainingBytes > 0) - hashCode.Add(reader.ReadByte()); - } - } - - private TypeDefinitionHandle FindType(string nameSpace, string name) - { - var hashCodeBuilder = default(HashCode); - AppendHashCode(ref hashCodeBuilder, nameSpace); - AppendHashCode(ref hashCodeBuilder, name); - - MetadataReader reader = _metadataReader; - MetadataStringComparer stringComparer = reader.StringComparer; - - TypeDefinitionHandle[] bucketHeads = _typeDefinitionBucketHeads ?? InitializeFindNamedType(); - TypeDefinitionHandle entry = bucketHeads[(uint)hashCodeBuilder.ToHashCode() % bucketHeads.Length]; - while (!entry.IsNil) - { - var typeDefinition = reader.GetTypeDefinition(entry); - if (stringComparer.Equals(typeDefinition.Name, name) && - stringComparer.Equals(typeDefinition.Namespace, nameSpace)) - { - return entry; - } - - entry = _typeDefinitionBuckets[MetadataTokens.GetRowNumber(entry)]; - } - - return default; - - static void AppendHashCodeNonAscii(ref HashCode hashCode, ReadOnlySpan s) - { - const int MaxStackAlloc = 1024; - int maxByteCount = Encoding.UTF8.GetMaxByteCount(s.Length); - Span utf8 = maxByteCount <= MaxStackAlloc ? stackalloc byte[MaxStackAlloc] : new byte[maxByteCount]; - int actualBytes = Encoding.UTF8.GetBytes(s, utf8); - - foreach (byte b in utf8.Slice(0, actualBytes)) - hashCode.Add(b); - } - - static void AppendHashCode(ref HashCode hashCode, string s) - { - for (int i = 0; i < s.Length; i++) - { - if (s[i] <= 0x7F) - { - hashCode.Add((byte)s[i]); - } - else - { - AppendHashCodeNonAscii(ref hashCode, ((ReadOnlySpan)s).Slice(i)); - return; - } - } - } - } - - public sealed override object GetType(string nameSpace, string name, NotFoundBehavior notFoundBehavior) { var currentModule = this; // src/coreclr/vm/clsload.cpp use the same restriction to detect a loop in the type forwarding. for (int typeForwardingChainSize = 0; typeForwardingChainSize <= 1024; typeForwardingChainSize++) { - TypeDefinitionHandle foundType = currentModule.FindType(nameSpace, name); - if (!foundType.IsNil) - return currentModule.GetType(foundType); - var metadataReader = currentModule._metadataReader; var stringComparer = metadataReader.StringComparer; + // TODO: More efficient implementation? + foreach (var typeDefinitionHandle in metadataReader.TypeDefinitions) + { + var typeDefinition = metadataReader.GetTypeDefinition(typeDefinitionHandle); + if (typeDefinition.Attributes.IsNested()) + continue; + + if (stringComparer.Equals(typeDefinition.Name, name) && + stringComparer.Equals(typeDefinition.Namespace, nameSpace)) + { + return currentModule.GetType(typeDefinitionHandle); + } + } foreach (var exportedTypeHandle in metadataReader.ExportedTypes) { From 156f22c8ab0dce3566d74042a1f7bee35ae3df2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 5 Apr 2023 13:54:29 +0900 Subject: [PATCH 4/5] Add named type lookup cache --- .../Common/TypeSystem/Ecma/EcmaModule.cs | 105 ++++++++++-------- 1 file changed, 56 insertions(+), 49 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs index 9a39af053bbea1..1c4d4c62ca591e 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs @@ -14,8 +14,8 @@ namespace Internal.TypeSystem.Ecma { public partial class EcmaModule : ModuleDesc { - private PEReader _peReader; - protected MetadataReader _metadataReader; + private readonly PEReader _peReader; + protected readonly MetadataReader _metadataReader; internal interface IEntityHandleObject { @@ -27,8 +27,8 @@ EntityHandle Handle private sealed class EcmaObjectLookupWrapper : IEntityHandleObject { - private EntityHandle _handle; - private object _obj; + private readonly EntityHandle _handle; + private readonly object _obj; public EcmaObjectLookupWrapper(EntityHandle handle, object obj) { @@ -55,7 +55,7 @@ public object Object internal sealed class EcmaObjectLookupHashtable : LockFreeReaderHashtable { - private EcmaModule _module; + private readonly EcmaModule _module; public EcmaObjectLookupHashtable(EcmaModule module) { @@ -178,8 +178,8 @@ private ModuleDesc ResolveModuleReference(ModuleReferenceHandle handle) return _moduleResolver.ResolveModule(this.Assembly, fileName); } - private LockFreeReaderHashtable _resolvedTokens; - private IModuleResolver _moduleResolver; + private readonly LockFreeReaderHashtable _resolvedTokens; + private readonly IModuleResolver _moduleResolver; internal EcmaModule(TypeSystemContext context, PEReader peReader, MetadataReader metadataReader, IAssemblyDesc containingAssembly, IModuleResolver customModuleResolver) : base(context, containingAssembly) @@ -282,23 +282,37 @@ public bool IsPlatformNeutral } } - private volatile Dictionary<(string Name, string Namespace), TypeDefinitionHandle> _lookup; + private Dictionary<(string Name, string Namespace), EntityHandle> _nameLookupCache; - private Dictionary<(string Name, string Namespace), TypeDefinitionHandle> CreateLookup() + private Dictionary<(string Name, string Namespace), EntityHandle> CreateNameLookupCache() { - var result = new Dictionary<(string Name, string Namespace), TypeDefinitionHandle>(); + // TODO: it's not particularly efficient to materialize strings just to hash them and hold + // onto them forever. We could instead hash the UTF-8 bytes and hold the TypeDefinitionHandle + // so we can obtain the bytes again when needed. + // E.g. see the scheme explored in the first commit of https://github.com/dotnet/runtime/pull/84285. - var metadataReader = _metadataReader; - foreach (var typeDefinitionHandle in metadataReader.TypeDefinitions) + var result = new Dictionary<(string Name, string Namespace), EntityHandle>(); + + MetadataReader metadataReader = _metadataReader; + foreach (TypeDefinitionHandle typeDefHandle in metadataReader.TypeDefinitions) { - var typeDefinition = metadataReader.GetTypeDefinition(typeDefinitionHandle); + TypeDefinition typeDefinition = metadataReader.GetTypeDefinition(typeDefHandle); if (typeDefinition.Attributes.IsNested()) continue; - result.Add((metadataReader.GetString(typeDefinition.Name), metadataReader.GetString(typeDefinition.Namespace)), typeDefinitionHandle); + result.Add((metadataReader.GetString(typeDefinition.Name), metadataReader.GetString(typeDefinition.Namespace)), typeDefHandle); } - return _lookup = result; + foreach (ExportedTypeHandle exportedTypeHandle in metadataReader.ExportedTypes) + { + ExportedType exportedType = metadataReader.GetExportedType(exportedTypeHandle); + if (exportedType.Implementation.Kind == HandleKind.ExportedType) + continue; + + result.Add((metadataReader.GetString(exportedType.Name), metadataReader.GetString(exportedType.Namespace)), exportedTypeHandle); + } + + return _nameLookupCache = result; } public sealed override object GetType(string nameSpace, string name, NotFoundBehavior notFoundBehavior) @@ -307,46 +321,39 @@ public sealed override object GetType(string nameSpace, string name, NotFoundBeh // src/coreclr/vm/clsload.cpp use the same restriction to detect a loop in the type forwarding. for (int typeForwardingChainSize = 0; typeForwardingChainSize <= 1024; typeForwardingChainSize++) { - if ((currentModule._lookup ?? currentModule.CreateLookup()).TryGetValue((name, nameSpace), out TypeDefinitionHandle typeDefHandle)) - return currentModule.GetType(typeDefHandle); - - var metadataReader = currentModule._metadataReader; - var stringComparer = metadataReader.StringComparer; - foreach (var exportedTypeHandle in metadataReader.ExportedTypes) + if ((currentModule._nameLookupCache ?? currentModule.CreateNameLookupCache()).TryGetValue((name, nameSpace), out EntityHandle foundHandle)) { - var exportedType = metadataReader.GetExportedType(exportedTypeHandle); - if (stringComparer.Equals(exportedType.Name, name) && - stringComparer.Equals(exportedType.Namespace, nameSpace)) + if (foundHandle.Kind == HandleKind.TypeDefinition) + return currentModule.GetType((TypeDefinitionHandle)foundHandle); + + ExportedType exportedType = currentModule._metadataReader.GetExportedType((ExportedTypeHandle)foundHandle); + if (exportedType.IsForwarder) { - if (exportedType.IsForwarder) - { - object implementation = currentModule.GetObject(exportedType.Implementation, notFoundBehavior); + object implementation = currentModule.GetObject(exportedType.Implementation, notFoundBehavior); - if (implementation == null) - { - return null; - } - if (implementation is EcmaModule ecmaModule) - { - currentModule = ecmaModule; - break; - } - if (implementation is ModuleDesc moduleDesc) - { - return moduleDesc.GetType(nameSpace, name, notFoundBehavior); - } - if (implementation is ResolutionFailure failure) - { - // No need to check notFoundBehavior - the callee already handled ReturnNull and Throw - return implementation; - } - // TODO - throw new NotImplementedException(); + if (implementation == null) + { + return null; } - - // TODO: + if (implementation is EcmaModule ecmaModule) + { + currentModule = ecmaModule; + } + if (implementation is ModuleDesc moduleDesc) + { + return moduleDesc.GetType(nameSpace, name, notFoundBehavior); + } + if (implementation is ResolutionFailure) + { + // No need to check notFoundBehavior - the callee already handled ReturnNull and Throw + return implementation; + } + // TODO throw new NotImplementedException(); } + + // TODO: + throw new NotImplementedException(); } } From 4cfc2e0d0999af52fe0ba2c5a1ea8ecff8c6400f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 5 Apr 2023 14:11:03 +0900 Subject: [PATCH 5/5] Oops --- .../Common/TypeSystem/Ecma/EcmaModule.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs index 1c4d4c62ca591e..738077dcd3f9b9 100644 --- a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs +++ b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs @@ -335,25 +335,30 @@ public sealed override object GetType(string nameSpace, string name, NotFoundBeh { return null; } - if (implementation is EcmaModule ecmaModule) + else if (implementation is EcmaModule ecmaModule) { currentModule = ecmaModule; } - if (implementation is ModuleDesc moduleDesc) + else if (implementation is ModuleDesc moduleDesc) { return moduleDesc.GetType(nameSpace, name, notFoundBehavior); } - if (implementation is ResolutionFailure) + else if (implementation is ResolutionFailure) { // No need to check notFoundBehavior - the callee already handled ReturnNull and Throw return implementation; } - // TODO + else + { + // TODO + throw new NotImplementedException(); + } + } + else + { + // TODO: throw new NotImplementedException(); } - - // TODO: - throw new NotImplementedException(); } }