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

Miscellaneous performance fixes for Crossgen2 #758

Merged
merged 5 commits into from
Dec 13, 2019
Merged

Miscellaneous performance fixes for Crossgen2 #758

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ace04d
Miscellaneous performance fixes.
Dec 11, 2019
40f6522
Fix indentation.
Dec 11, 2019
964ec9d
Address some PR comments.
Dec 11, 2019
be9864f
Remove EnumExtensions file.
Dec 12, 2019
f62bc4a
Fix determinism issue. Fix map file flag passing.
Dec 12, 2019
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
76 changes: 42 additions & 34 deletions src/coreclr/src/tools/Common/Compiler/CoreRTNameMangler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Security.Cryptography;
using System.Text;

Expand Down Expand Up @@ -144,7 +143,7 @@ private string SanitizeNameWithHash(string literal)
/// <summary>
/// Dictionary given a mangled name for a given <see cref="TypeDesc"/>
/// </summary>
private ImmutableDictionary<TypeDesc, string> _mangledTypeNames = ImmutableDictionary<TypeDesc, string>.Empty;
private Dictionary<TypeDesc, string> _mangledTypeNames = new Dictionary<TypeDesc, string>();

/// <summary>
/// Given a set of names <param name="set"/> check if <param name="origName"/>
Expand All @@ -166,11 +165,14 @@ private string DisambiguateName(string origName, ISet<string> set)

public override string GetMangledTypeName(TypeDesc type)
{
string mangledName;
if (_mangledTypeNames.TryGetValue(type, out mangledName))
return mangledName;
lock (this)
Copy link
Member

Choose a reason for hiding this comment

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

Can we break down the locking hierarchy so that there's not a single giant lock for type names, method names, field names, and mangled strings, but instead there's a lock for each?

In general, I'm worried that we're probably not using this from multithreaded code at all right now, so this looks like an improvement, but the locks will become a bottleneck as soon as we start doing that.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about using the lock free concurrent dictionaries here, to avoid locking while performing the lookup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichalStrehovsky We are using this from multithreaded code at the moment. I tried using more granulated locking and it didn't seem to make a performance difference. @davidwrighton and I thought that, since there's no performance difference right now, it would be better to keep it in this form where it's clear to people reading it that there are no deadlocks.

@fadimounir That's an option, but then it gets a little hard to analyze because it's hard to reason about whether each of the GetOrAdd functions must only be run once and so has to be either locked or wrapped in a Lazy. This + the limited time left is why we didn't end up using that right now, but perhaps @davidwrighton can provide more direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

because it's hard to reason about whether each of the GetOrAdd functions must only be run once

What do you mean? We can always do a .TryGetValue outside of a lock, and .TryAdd inside a lock (Dictionary has the same APIs too so we can use it as well). Locking while retrieving a value is not ideal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see what you mean now. Yes I could change it to do that, but I'm not sure whether we even have contention there.

Copy link
Member

Choose a reason for hiding this comment

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

When using Dictionary you cannot use the TryGetValue apis outside of a lock. This worked before with ImmutableDictionary as that api allows lock-free reads. My expectation is that ConcurrentDictionary would be better in performance here than Dictionary, but I don't want to see us use that without seeing the performance impact in a profiler. I think we should check Anubhav's work in here as written, and explore finding even more performance in the future.

{
string mangledName;
if (_mangledTypeNames.TryGetValue(type, out mangledName))
return mangledName;

return ComputeMangledTypeName(type);
return ComputeMangledTypeName(type);
}
}

private string EnterNameScopeSequence => _mangleForCplusPlus ? "_A_" : "<";
Expand Down Expand Up @@ -273,12 +275,11 @@ private string ComputeMangledTypeName(TypeDesc type)
// Ensure that name is unique and update our tables accordingly.
name = DisambiguateName(name, deduplicator);
deduplicator.Add(name);
_mangledTypeNames = _mangledTypeNames.Add(t, name);
_mangledTypeNames.Add(t, name);
}
}
return _mangledTypeNames[type];
}

return _mangledTypeNames[type];
}

string mangledName;
Expand Down Expand Up @@ -351,14 +352,14 @@ private string ComputeMangledTypeName(TypeDesc type)
{
// Ensure that name is unique and update our tables accordingly.
if (!_mangledTypeNames.ContainsKey(type))
_mangledTypeNames = _mangledTypeNames.Add(type, mangledName);
_mangledTypeNames.Add(type, mangledName);
}

return mangledName;
}

private ImmutableDictionary<MethodDesc, Utf8String> _mangledMethodNames = ImmutableDictionary<MethodDesc, Utf8String>.Empty;
private ImmutableDictionary<MethodDesc, Utf8String> _unqualifiedMangledMethodNames = ImmutableDictionary<MethodDesc, Utf8String>.Empty;
private Dictionary<MethodDesc, Utf8String> _mangledMethodNames = new Dictionary<MethodDesc, Utf8String>();
private Dictionary<MethodDesc, Utf8String> _unqualifiedMangledMethodNames = new Dictionary<MethodDesc, Utf8String>();

public override Utf8String GetMangledMethodName(MethodDesc method)
{
Expand All @@ -381,7 +382,7 @@ public override Utf8String GetMangledMethodName(MethodDesc method)
lock (this)
{
if (!_mangledMethodNames.ContainsKey(method))
_mangledMethodNames = _mangledMethodNames.Add(method, utf8MangledName);
_mangledMethodNames.Add(method, utf8MangledName);
}

return utf8MangledName;
Expand All @@ -390,11 +391,14 @@ public override Utf8String GetMangledMethodName(MethodDesc method)

private Utf8String GetUnqualifiedMangledMethodName(MethodDesc method)
{
Utf8String mangledName;
if (_unqualifiedMangledMethodNames.TryGetValue(method, out mangledName))
return mangledName;
lock (this)
{
Utf8String mangledName;
if (_unqualifiedMangledMethodNames.TryGetValue(method, out mangledName))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here... if we make _unqualifiedMangledMethodNames a lock-free dictionary, we can avoid locking while looking up. We only need locking while adding new entries

return mangledName;

return ComputeUnqualifiedMangledMethodName(method);
return ComputeUnqualifiedMangledMethodName(method);
}
}

private Utf8String GetPrefixMangledTypeName(IPrefixMangledType prefixMangledType)
Expand Down Expand Up @@ -481,12 +485,11 @@ private Utf8String ComputeUnqualifiedMangledMethodName(MethodDesc method)
name = DisambiguateName(name, deduplicator);
deduplicator.Add(name);

_unqualifiedMangledMethodNames = _unqualifiedMangledMethodNames.Add(m, name);
_unqualifiedMangledMethodNames.Add(m, name);
}
}
return _unqualifiedMangledMethodNames[method];
}

return _unqualifiedMangledMethodNames[method];
}

Utf8String utf8MangledName;
Expand Down Expand Up @@ -550,22 +553,25 @@ private Utf8String ComputeUnqualifiedMangledMethodName(MethodDesc method)
lock (this)
{
if (!_unqualifiedMangledMethodNames.ContainsKey(method))
_unqualifiedMangledMethodNames = _unqualifiedMangledMethodNames.Add(method, utf8MangledName);
_unqualifiedMangledMethodNames.Add(method, utf8MangledName);
}
}

return utf8MangledName;
}

private ImmutableDictionary<FieldDesc, Utf8String> _mangledFieldNames = ImmutableDictionary<FieldDesc, Utf8String>.Empty;
private Dictionary<FieldDesc, Utf8String> _mangledFieldNames = new Dictionary<FieldDesc, Utf8String>();

public override Utf8String GetMangledFieldName(FieldDesc field)
{
Utf8String mangledName;
if (_mangledFieldNames.TryGetValue(field, out mangledName))
return mangledName;
lock (this)
{
Utf8String mangledName;
if (_mangledFieldNames.TryGetValue(field, out mangledName))
return mangledName;

return ComputeMangledFieldName(field);
return ComputeMangledFieldName(field);
}
}

private Utf8String ComputeMangledFieldName(FieldDesc field)
Expand Down Expand Up @@ -594,12 +600,11 @@ private Utf8String ComputeMangledFieldName(FieldDesc field)
if (prependTypeName != null)
name = prependTypeName + "__" + name;

_mangledFieldNames = _mangledFieldNames.Add(f, name);
_mangledFieldNames.Add(f, name);
}
}
return _mangledFieldNames[field];
}

return _mangledFieldNames[field];
}


Expand All @@ -613,26 +618,29 @@ private Utf8String ComputeMangledFieldName(FieldDesc field)
lock (this)
{
if (!_mangledFieldNames.ContainsKey(field))
_mangledFieldNames = _mangledFieldNames.Add(field, utf8MangledName);
_mangledFieldNames.Add(field, utf8MangledName);
}

return utf8MangledName;
}

private ImmutableDictionary<string, string> _mangledStringLiterals = ImmutableDictionary<string, string>.Empty;
private Dictionary<string, string> _mangledStringLiterals = new Dictionary<string, string>();

public override string GetMangledStringName(string literal)
{
string mangledName;
if (_mangledStringLiterals.TryGetValue(literal, out mangledName))
return mangledName;
lock (this)
{
if (_mangledStringLiterals.TryGetValue(literal, out mangledName))
return mangledName;
}

mangledName = SanitizeNameWithHash(literal);

lock (this)
{
if (!_mangledStringLiterals.ContainsKey(literal))
_mangledStringLiterals = _mangledStringLiterals.Add(literal, mangledName);
_mangledStringLiterals.Add(literal, mangledName);
}

return mangledName;
Expand Down
37 changes: 37 additions & 0 deletions src/coreclr/src/tools/Common/System/EnumExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

Copy link
Member

Choose a reason for hiding this comment

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

I can see that you removed usage of these apis, but this file seems to still be here. Please remove this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh oops, yes.

using ILCompiler.DependencyAnalysis.ReadyToRun;
using Internal.JitInterface;
using Internal.ReadyToRunConstants;

namespace System
{
/// <summary>
/// Enum.CompareTo takes an Object, so using that requires boxing the enum we are comparing to (which is slow
Copy link
Member

Choose a reason for hiding this comment

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

I do not think it makes sense to have these in the shared file.

Instead, I would just add casts to int to every place where this is used (like what I have suggested below).

/// and allocates unnecessarily).
/// </summary>
internal static class EnumExtensions
{
public static int CompareEnum(this ReadyToRunFixupKind first, ReadyToRunFixupKind second)
{
return (int)first - (int)second;
}

public static int CompareEnum(this ImportThunk.Kind first, ImportThunk.Kind second)
Copy link
Member

Choose a reason for hiding this comment

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

If the type is not in Common, code using the type should not be in Common.

+1 to Jan's advice.

{
return (int)first - (int)second;
}

public static int CompareEnum(this mdToken first, mdToken second)
{
return (int)first - (int)second;
}

public static int CompareEnum(this CORINFO_RUNTIME_LOOKUP_KIND first, CORINFO_RUNTIME_LOOKUP_KIND second)
{
return (int)first - (int)second;
}
}
}
13 changes: 7 additions & 6 deletions ...ssgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/CopiedFieldRvaNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Immutable;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Reflection.PortableExecutable;
using Internal.Text;
using Internal.TypeSystem;
using Internal.TypeSystem.Ecma;
Expand Down Expand Up @@ -61,8 +62,6 @@ private unsafe byte[] GetRvaData(int targetPointerSize)
BlobReader metadataBlob = new BlobReader(_module.PEReader.GetMetadata().Pointer, _module.PEReader.GetMetadata().Length);
metadataBlob.Offset = metadataReader.GetTableMetadataOffset(TableIndex.FieldRva);

ImmutableArray<byte> memBlock = _module.PEReader.GetSectionData(_rva).GetContent();

for (int i = 1; i <= metadataReader.GetTableRowCount(TableIndex.FieldRva); i++)
{
int currentFieldRva = metadataBlob.ReadInt32();
Expand All @@ -76,18 +75,20 @@ private unsafe byte[] GetRvaData(int targetPointerSize)
int currentSize = field.FieldType.GetElementSize().AsInt;
if (currentSize > size)
{
if (currentSize > memBlock.Length)
throw new BadImageFormatException();

// We need to handle overlapping fields by reusing blobs based on the rva, and just update
// the size and contents
size = currentSize;
result = new byte[AlignmentHelper.AlignUp(size, targetPointerSize)];
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also move the array allocation outside of the loop. No need to allocate and resize now

memBlock.CopyTo(0, result, 0, size);
}
}

Debug.Assert(size > 0);

PEMemoryBlock block = _module.PEReader.GetSectionData(_rva);
if (block.Length < size)
throw new BadImageFormatException();

block.GetContent(0, size).CopyTo(result);
return result;
}

Expand Down
2 changes: 1 addition & 1 deletion ...sgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilde
public override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
{
FieldFixupSignature otherNode = (FieldFixupSignature)other;
int result = _fixupKind.CompareTo(otherNode._fixupKind);
int result = _fixupKind.CompareEnum(otherNode._fixupKind);
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
int result = _fixupKind.CompareEnum(otherNode._fixupKind);
int result = ((int)_fixupKind).CompareTo((int)otherNode._fixupKind);

Copy link
Member

Choose a reason for hiding this comment

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

Why not just _fixupKind - otherNode._fixupKind ?

Copy link
Contributor

@fadimounir fadimounir Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No need to call CompareTo. Just do the subtraction here directly as @huoyaoyuan suggests (and typecast if necessary).

Applies to all other places

Copy link
Member

Choose a reason for hiding this comment

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

Note that the subtraction does not work to compare arbitrary integers. It only works if the integers are small. It is probably the case here, but then you need to think about all the time about whether you are dealing with small integers or not.

Copy link
Contributor

@lpereira lpereira Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Might be an opportunity to improve the codegen for int.CompareTo(). It's currently not branch-free. For instance, these two variants that works with arbitrary integers could be used instead of the branchy version:

  • Convert.ToInt32(a > b) - Convert.ToInt32(a < b) should be something like xor eax, eax; xor ecx,ecx; cmp edi, esi; seg al; setl cl; sub eax, ecx

(Might also be able to use a conditional move, although it doesn't seem that necessary.)

if (result != 0)
return result;

Expand Down
4 changes: 2 additions & 2 deletions ...n2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GenericLookupSignature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,11 @@ public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilde
public override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
{
GenericLookupSignature otherNode = (GenericLookupSignature)other;
int result = _runtimeLookupKind.CompareTo(otherNode._runtimeLookupKind);
int result = _runtimeLookupKind.CompareEnum(otherNode._runtimeLookupKind);
if (result != 0)
return result;

result = _fixupKind.CompareTo(otherNode._fixupKind);
result = _fixupKind.CompareEnum(otherNode._fixupKind);
if (result != 0)
return result;

Expand Down
6 changes: 4 additions & 2 deletions ...ols/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ImportThunk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;

using Internal.Text;
using Internal.ReadyToRunConstants;

Expand All @@ -13,7 +15,7 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun
/// </summary>
public partial class ImportThunk : AssemblyStubNode, ISymbolDefinitionNode
{
enum Kind
public enum Kind
{
Eager,
Lazy,
Expand Down Expand Up @@ -74,7 +76,7 @@ protected override string GetName(NodeFactory factory)
public override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
{
ImportThunk otherNode = (ImportThunk)other;
int result = _thunkKind.CompareTo(otherNode._thunkKind);
int result = _thunkKind.CompareEnum(otherNode._thunkKind);
if (result != 0)
return result;

Expand Down
2 changes: 1 addition & 1 deletion ...gen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilde
public override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
{
MethodFixupSignature otherNode = (MethodFixupSignature)other;
int result = _fixupKind.CompareTo(otherNode._fixupKind);
int result = _fixupKind.CompareEnum(otherNode._fixupKind);
if (result != 0)
return result;

Expand Down
4 changes: 2 additions & 2 deletions ...ols/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public bool Equals(ModuleToken moduleToken)

public int CompareTo(ModuleToken other)
{
int result = Token.CompareTo(other.Token);
int result = Token.CompareEnum(other.Token);
if (result != 0)
return result;

Expand All @@ -79,7 +79,7 @@ public SignatureContext SignatureContext(ModuleTokenResolver resolver)
return new SignatureContext(Module, resolver);
}

public MetadataReader MetadataReader => Module.PEReader.GetMetadataReader();
public MetadataReader MetadataReader => Module.MetadataReader;

public CorTokenType TokenType => SignatureBuilder.TypeFromToken(Token);

Expand Down
2 changes: 1 addition & 1 deletion ...ssgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilde
public override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
{
TypeFixupSignature otherNode = (TypeFixupSignature)other;
int result = _fixupKind.CompareTo(otherNode._fixupKind);
int result = _fixupKind.CompareEnum(otherNode._fixupKind);
if (result != 0)
return result;

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
<Compile Include="..\..\Common\Internal\NativeFormat\NativeFormat.cs" Link="Common\NativeFormat.cs" />
<Compile Include="..\..\Common\Internal\NativeFormat\NativeFormatWriter.cs" Link="Common\NativeFormatWriter.cs" />
<Compile Include="..\..\Common\Internal\NativeFormat\NativeFormatWriter.Primitives.cs" Link="Common\NativeFormatWriter.Primitives.cs" />
<Compile Include="..\..\Common\System\EnumExtensions.cs" Link="Common\EnumExtensions.cs" />
<Compile Include="..\..\Common\System\FormattingHelpers.cs" Link="Common\FormattingHelpers.cs" />
<Compile Include="..\..\Common\TypeSystem\IL\ILProvider.cs" Link="IL\ILProvider.cs" />
<Compile Include="..\..\Common\TypeSystem\IL\Stubs\ComparerIntrinsics.cs" Link="IL\Stubs\ComparerIntrinsics.cs" />
Expand Down