Miscellaneous performance fixes for Crossgen2 #758

SrivastavaAnubhav · 2019-12-11T02:24:17Z

Overall, these changes speed up Crossgen2 compilation of CoreLib by ~34%. Most of the improvement comes from fewer/smaller allocations => need to do less GC.

Create comparison functions for Enum (Enum.CompareTo boxes since it takes an Object)
In ModuleToken, use the existing MetadataReader instead of making a new one each time
Use Dictionary instead of ImmutableDictionary in CoreRTNameMangler to reduce allocations
In CopiedFieldRvaNode, only read as much of the section as we need to copy

Before changes:

Total average compilation time: 9522.11 ms
Phase breakdown (average):
- Loading time: 682.91 ms
- Graph processing time: 4189.59 ms
  - Added 211462 nodes to mark
  - Dependency analysis time: 622.58
  - Wall clock JIT time: 2988.49
  - Total JIT time: 23079.96 ms (sum of all threads)
  - 22680 methods JITed
- Emitting time: 4646.94 ms

After changes:

Total average compilation time: 6245.38 ms
Phase breakdown (average):
- Loading time: 697.13 ms
- Graph processing time: 3670.02 ms
  - Added 211462 nodes to mark stack
  - Dependency analysis time: 631.47 ms
  - Wall clock JIT time: 2483.27 ms
  - Total JIT time: 19068.50 ms (sum of all threads)
  - 22680 methods JITed
- Emitting time: 1875.50 ms

Create comparison functions for Enum (Enum.CompareTo boxes since it takes an Object). In ModuleToken, use the existing MetadataReader instead of making a new one each time. Use Dictionary instead of ImmutableDictionary in CoreRTNameMangler to reduce allocations. In CopiedFieldRvaNode, only read as much of the section as we need to copy.

SrivastavaAnubhav · 2019-12-11T02:25:59Z

cc @dotnet/crossgen-contrib

jkotas · 2019-12-11T02:32:17Z

...rossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs

@@ -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);


Suggested change

int result = _fixupKind.CompareEnum(otherNode._fixupKind);

int result = ((int)_fixupKind).CompareTo((int)otherNode._fixupKind);

Why not just _fixupKind - otherNode._fixupKind ?

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

Applies to all other places

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.

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.)

jkotas · 2019-12-11T02:32:43Z

src/coreclr/src/tools/Common/System/EnumExtensions.cs

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


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).

MichalStrehovsky · 2019-12-11T05:42:43Z

src/coreclr/src/tools/Common/Compiler/CoreRTNameMangler.cs

-            string mangledName;
-            if (_mangledTypeNames.TryGetValue(type, out mangledName))
-                return mangledName;
+            lock (this)


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.

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

@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.

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

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.

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.

MichalStrehovsky · 2019-12-11T05:44:09Z

src/coreclr/src/tools/Common/System/EnumExtensions.cs

+            return (int)first - (int)second;
+        }
+
+        public static int CompareEnum(this ImportThunk.Kind first, ImportThunk.Kind second)


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

+1 to Jan's advice.

fadimounir · 2019-12-11T16:31:23Z

src/coreclr/src/tools/Common/Compiler/CoreRTNameMangler.cs

+            lock (this)
+            {
+                Utf8String mangledName;
+                if (_unqualifiedMangledMethodNames.TryGetValue(method, out mangledName))


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

fadimounir · 2019-12-11T16:34:07Z

...crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/CopiedFieldRvaNode.cs

@@ -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)];


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

fadimounir · 2019-12-11T16:37:17Z

The new perf numbers look pretty nice! Thank you!

Remove EnumExtensions in favor of casting to int and using CompareTo. Move array allocation out of loop in CopiedFieldRvaNode.

davidwrighton · 2019-12-12T00:50:14Z

src/coreclr/src/tools/Common/System/EnumExtensions.cs

+// 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.
+


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

Oh oops, yes.

fadimounir · 2019-12-12T18:09:48Z

@SrivastavaAnubhav, these changes are breaking determinism. See: https://dev.azure.com/dnceng/public/_build/results?buildId=454679&view=ms.vss-test-web.build-test-results-tab&runId=14512252&resultId=102657&paneView=attachments

Dotnet-GitSync-Bot added the area-crossgen2-coreclr label Dec 11, 2019

Fix indentation.

40f6522

jkotas reviewed Dec 11, 2019

View reviewed changes

MichalStrehovsky reviewed Dec 11, 2019

View reviewed changes

fadimounir reviewed Dec 11, 2019

View reviewed changes

Address some PR comments.

964ec9d

Remove EnumExtensions in favor of casting to int and using CompareTo. Move array allocation out of loop in CopiedFieldRvaNode.

SrivastavaAnubhav force-pushed the perf-improvements branch from 2a33bb3 to 964ec9d Compare December 11, 2019 19:07

davidwrighton reviewed Dec 12, 2019

View reviewed changes

Remove EnumExtensions file.

be9864f

fadimounir approved these changes Dec 12, 2019

View reviewed changes

Fix determinism issue. Fix map file flag passing.

f62bc4a

fadimounir merged commit d850517 into dotnet:master Dec 13, 2019

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous performance fixes for Crossgen2 #758

Miscellaneous performance fixes for Crossgen2 #758

SrivastavaAnubhav commented Dec 11, 2019

SrivastavaAnubhav commented Dec 11, 2019

jkotas Dec 11, 2019

huoyaoyuan Dec 11, 2019

fadimounir Dec 11, 2019 •

edited

Loading

jkotas Dec 11, 2019

lpereira Dec 12, 2019 •

edited

Loading

jkotas Dec 11, 2019

MichalStrehovsky Dec 11, 2019

fadimounir Dec 11, 2019

SrivastavaAnubhav Dec 11, 2019

fadimounir Dec 11, 2019

SrivastavaAnubhav Dec 11, 2019

davidwrighton Dec 12, 2019

MichalStrehovsky Dec 11, 2019

fadimounir Dec 11, 2019

fadimounir Dec 11, 2019

fadimounir commented Dec 11, 2019

davidwrighton Dec 12, 2019

SrivastavaAnubhav Dec 12, 2019

fadimounir commented Dec 12, 2019

	int result = _fixupKind.CompareEnum(otherNode._fixupKind);
	int result = ((int)_fixupKind).CompareTo((int)otherNode._fixupKind);

Miscellaneous performance fixes for Crossgen2 #758

Miscellaneous performance fixes for Crossgen2 #758

Conversation

SrivastavaAnubhav commented Dec 11, 2019

SrivastavaAnubhav commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fadimounir Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fadimounir commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fadimounir commented Dec 12, 2019

fadimounir Dec 11, 2019 •

edited

Loading

lpereira Dec 12, 2019 •

edited

Loading