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.

Sign up for GitHub

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

Jump to bottom

Speed up named type lookups in managed type system #84285

Merged
merged 6 commits into from
Apr 5, 2023
Merged

Speed up named type lookups in managed type system #84285

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2e40a84
Speed up named type lookups
MichalStrehovsky Apr 4, 2023
a49ec78
Naïve cache
MichalStrehovsky Apr 5, 2023
a5d9788
Revert "Speed up named type lookups"
MichalStrehovsky Apr 5, 2023
214e6ff
Merge branch 'naive' into gettypecache
MichalStrehovsky Apr 5, 2023
156f22c
Add named type lookup cache
MichalStrehovsky Apr 5, 2023
4cfc2e0
Oops
MichalStrehovsky Apr 5, 2023
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
114 changes: 67 additions & 47 deletions src/coreclr/tools/Common/TypeSystem/Ecma/EcmaModule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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)
{
Expand All @@ -55,7 +55,7 @@ public object Object

internal sealed class EcmaObjectLookupHashtable : LockFreeReaderHashtable<EntityHandle, IEntityHandleObject>
{
private EcmaModule _module;
private readonly EcmaModule _module;

public EcmaObjectLookupHashtable(EcmaModule module)
{
Expand Down Expand Up @@ -178,8 +178,8 @@ private ModuleDesc ResolveModuleReference(ModuleReferenceHandle handle)
return _moduleResolver.ResolveModule(this.Assembly, fileName);
}

private LockFreeReaderHashtable<EntityHandle, IEntityHandleObject> _resolvedTokens;
private IModuleResolver _moduleResolver;
private readonly LockFreeReaderHashtable<EntityHandle, IEntityHandleObject> _resolvedTokens;
private readonly IModuleResolver _moduleResolver;

internal EcmaModule(TypeSystemContext context, PEReader peReader, MetadataReader metadataReader, IAssemblyDesc containingAssembly, IModuleResolver customModuleResolver)
: base(context, containingAssembly)
Expand Down Expand Up @@ -282,60 +282,80 @@ public bool IsPlatformNeutral
}
}

private Dictionary<(string Name, string Namespace), EntityHandle> _nameLookupCache;

private Dictionary<(string Name, string Namespace), EntityHandle> CreateNameLookupCache()
{
// 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 result = new Dictionary<(string Name, string Namespace), EntityHandle>();
Copy link
Member

@jkotas jkotas Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we care enough, this can be made more efficient by:

  • Pre-sizing the dictionary - either conservatively using total counts of TypeDefinitionHandle and ExportedTypeHandle tables, or precisely by doing a quick pass over these tables to see how many entries we are actually going to need.
  • Using a custom struct with faster non-randomized GetHashCode algorithm (e.g. use the FNV hashcode implementation from Roslyn)

Copy link
Member

Choose a reason for hiding this comment

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

Pre-sizing the dictionary can help a lot with GC throughput in my experience. That may matter in the case that we keep the code that does a GC before handing off to the object writer.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we go the custom hasher route, it's not that much extra work to do the thing I did in the first commit and also get rid of the dictionary in the first place.

We could also explore FrozenDictionary although it probably won't work for netstandard2.0 right now.

I don't have big concerns based on the perf numbers I collected. This is an improvement even when not reading MIBC (the perf numbers with Dictionary were collected without MIBC).


MetadataReader metadataReader = _metadataReader;
foreach (TypeDefinitionHandle typeDefHandle in metadataReader.TypeDefinitions)
{
TypeDefinition typeDefinition = metadataReader.GetTypeDefinition(typeDefHandle);
if (typeDefinition.Attributes.IsNested())
continue;

result.Add((metadataReader.GetString(typeDefinition.Name), metadataReader.GetString(typeDefinition.Namespace)), typeDefHandle);
}

foreach (ExportedTypeHandle exportedTypeHandle in metadataReader.ExportedTypes)
{
ExportedType exportedType = metadataReader.GetExportedType(exportedTypeHandle);
if (exportedType.Implementation.Kind == HandleKind.ExportedType)
Copy link
Member Author

Choose a reason for hiding this comment

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

This line is a bugfix.

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)
{
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++)
{
var metadataReader = currentModule._metadataReader;
var stringComparer = metadataReader.StringComparer;
// TODO: More efficient implementation?
foreach (var typeDefinitionHandle in metadataReader.TypeDefinitions)
if ((currentModule._nameLookupCache ?? currentModule.CreateNameLookupCache()).TryGetValue((name, nameSpace), out EntityHandle foundHandle))
{
var typeDefinition = metadataReader.GetTypeDefinition(typeDefinitionHandle);
if (typeDefinition.Attributes.IsNested())
continue;
if (foundHandle.Kind == HandleKind.TypeDefinition)
return currentModule.GetType((TypeDefinitionHandle)foundHandle);

if (stringComparer.Equals(typeDefinition.Name, name) &&
stringComparer.Equals(typeDefinition.Namespace, nameSpace))
ExportedType exportedType = currentModule._metadataReader.GetExportedType((ExportedTypeHandle)foundHandle);
if (exportedType.IsForwarder)
{
return currentModule.GetType(typeDefinitionHandle);
}
}
object implementation = currentModule.GetObject(exportedType.Implementation, notFoundBehavior);

foreach (var exportedTypeHandle in metadataReader.ExportedTypes)
{
var exportedType = metadataReader.GetExportedType(exportedTypeHandle);
if (stringComparer.Equals(exportedType.Name, name) &&
stringComparer.Equals(exportedType.Namespace, nameSpace))
{
if (exportedType.IsForwarder)
if (implementation == null)
{
return null;
}
else if (implementation is EcmaModule ecmaModule)
{
currentModule = ecmaModule;
}
else if (implementation is ModuleDesc moduleDesc)
{
return moduleDesc.GetType(nameSpace, name, notFoundBehavior);
}
else if (implementation is ResolutionFailure)
{
// No need to check notFoundBehavior - the callee already handled ReturnNull and Throw
return implementation;
}
else
{
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();
}

}
else
{
// TODO:
throw new NotImplementedException();
}
Expand Down