Compile System.Private.CoreLib with Crossgen2 by default #44618

trylek · 2020-11-12T23:45:29Z

After I deleted the Utf8String-related experimental code from the
runtime repo per JanK's suggestion, I can now enable Crossgen2
compilation for System.Private.CoreLib by default.

Thanks

Tomas

/cc: @dotnet/crossgen-contrib

stephentoub · 2020-11-12T23:47:41Z

Does this have an impact on code quality, or is the before/after identical?

trylek · 2020-11-12T23:52:48Z

Our measurements so far indicate that Crossgen- and Crossgen2-compiled SPC should be almost identical with regards to runtime performance, working set and size on disk. Having said that, merging this commit will throw a much broader set of tests at the library so I cannot rule out some outlier popping up, that's why this change is intentionally trivial to revert. @mangod9 also suggested that I shoot a heads-up e-mail to the runtime repo committers once this goes in to help identify the cause of any issues that may appear.

stephentoub · 2020-11-13T00:10:45Z

Ok, thanks.

mangod9 · 2020-11-13T00:24:13Z

There appears to be a build failure on OSX?

janvorli · 2020-11-13T00:29:39Z

The Linux x64 crossgen2 invocation failed. It has failed with an assert:

 Process terminated. Assertion failed.
  Please update intrinsic hash table
     at Internal.JitInterface.CorInfoImpl.InitializeIntrinsicHashtable() in /_/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.Intrinsics.cs:line 115
     at Internal.JitInterface.CorInfoImpl..cctor() in /_/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.Intrinsics.cs:line 120
     at Internal.JitInterface.CorInfoImpl.ObjectToHandle(Object obj) in /_/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs:line 421
     at Internal.JitInterface.CorInfoImpl.ObjectToHandle(MethodDesc method) in /_/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs:line 439
     at Internal.JitInterface.CorInfoImpl.Get_CORINFO_METHOD_INFO(MethodDesc method, MethodIL methodIL, CORINFO_METHOD_INFO* methodInfo) in /_/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs:line 455
     at Internal.JitInterface.CorInfoImpl.CompileMethodInternal(IMethodNode methodCodeNodeNeedingCode, MethodIL methodIL) in /_/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs:line 147
     at Internal.JitInterface.CorInfoImpl.CompileMethod(MethodWithGCInfo methodCodeNodeNeedingCode) in /_/src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs:line 391
     at ILCompiler.ReadyToRunCodegenCompilation.<ComputeDependencyNodeDependencies>b__26_0(DependencyNodeCore`1 dependency) in /_/src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs:line 500
     at System.Threading.Tasks.Parallel.<>c__DisplayClass33_0`2.<ForEachWorker>b__0(Int32 i)
     at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`1.<ForWorker>b__1(RangeWorker& currentWorker, Int32 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
     at System.Threading.Tasks.TaskReplicator.Replica`1.ExecuteAction(Boolean& yieldedBeforeCompletion)
     at System.Threading.Tasks.TaskReplicator.Replica.Execute()
     at System.Threading.Tasks.TaskReplicator.Replica.<>c.<.ctor>b__4_0(Object s)
     at System.Threading.Tasks.Task.InnerInvoke()
     at System.Threading.Tasks.Task.<>c.<.cctor>b__277_0(Object obj)
     at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
     at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
     at System.Threading.Tasks.Task.ExecuteEntryUnsafe(Thread threadPoolThread)
     at System.Threading.Tasks.Task.ExecuteFromThreadPool(Thread threadPoolThread)
     at System.Threading.ThreadPoolWorkQueue.Dispatch()
     at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback()

janvorli · 2020-11-13T00:30:34Z

The win x86 and OSX x64 too

trylek · 2020-11-13T01:34:42Z

Got it, @EgorBo's change

819a3cc#diff-ff19628c24a151a3f919c197c4785be0c9b10fa61f2e61beb4e4867b676dcc8c

removed two intrinsics without updating the assertion check at

runtime/src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.Intrinsics.cs

Line 115 in 4c102bd

    
           Debug.Assert((int)CorInfoIntrinsics.CORINFO_INTRINSIC_Count == 34, "Please update intrinsic hash table");

I'll send out the fix as a separate PR to keep this switch-over PR trivial to rollback.

trylek · 2020-11-13T01:41:46Z

PR is out: #44624

AndyAyersMS · 2020-11-13T01:44:05Z

Before you merge, it would be good for @DrewScoggins to verify we're not seeing any perf lab outage, so if we see any regressions we have a dense set of runs to help pin down which change is responsible.

trylek · 2020-11-13T01:47:27Z

I've booked vacation from Friday till next Tuesday. Assuming I manage to check in the fix for the assertion check tomorrow morning before I leave, I would be probably inclined to check this in on Saturday or Sunday - this should hopefully also make it easier to identify potential fallout of this change as the rate of check-ins is usually lower over the weekend.

…ng purposes

trylek · 2020-11-13T01:55:41Z

Currently lab testing in an instrumented run including the fix for the assertion check: #44625

AndyAyersMS · 2020-11-13T02:15:29Z

cc @dotnet/jit-contrib

Please make sure there's a good writeup somewhere on how to repro and debug crossgen2 errors (particularly what happens if the jit asserts or crashes, and how to pass complus settings), as the first problems a jit developer often sees with new jit code surface during the crossgen of SPC that's done by the build.

We also need to make sure PMI, SPMI, and our automated SPMI collection processes are ready. @sandreenko weren't you looking at this recently?

mangod9 · 2020-11-13T05:11:05Z

@AndyAyersMS crossgen2 has been validated with various weekly CI jobs, this change essentially now compiles s.p.corelib.dll by default. Is your concern mainly that there are going to be some JIT failures which might be difficult to debug? Regardless we can certainly get a runbook created for debugging steps.

MichalStrehovsky · 2020-11-13T09:03:22Z

The Linux x64 crossgen2 invocation failed. It has failed with an assert:

Btw, I still have #43017 out to make the Checked build actually different from a Release build and have asserts fire consistently. I need a signoff to merge, but I guess we need to fix the regression first.

DrewScoggins · 2020-11-13T17:26:36Z

@trylek Could you just comment out the line pr: none in https://github.com/dotnet/runtime/blob/master/eng/pipelines/coreclr/perf.yml. This way we can run through the performance tests to make sure that none of them are broken by this change. You can then remove the comment before you check-in.

mangod9 · 2020-11-13T17:56:05Z

@DrewScoggins, I have pushed the change to run perf scenarios.

AndyAyersMS · 2020-11-13T18:58:11Z

Is your concern mainly that there are going to be some JIT failures which might be difficult to debug

@mangod9 because the newly built jit runs during the crossgen steps of the build, changes to the jit can cause build failures. So often the first debugging one does as part of changing the jit is to fix the issues that arise during crossgen. You often have to do this before running simple tests, if that step of the build can't easily be bypassed. Since the crossgen run is kicked off by the build it may not be easy to figure out how to repro and understand these failures by say running the same thing under the debugger.

Thus it's essential that the debugging experience be well documented, eg what command lines are needed, how to specify which jit is being invoked, how to tunnel through the complus settings that are useful when investigating jit issues.

We also often use crossgen for estimating throughput of jit changes. I suspect this is not going to work very well anymore with crossgen2, but we'll have to see.

mangod9 · 2020-11-13T19:27:43Z

Thus it's essential that the debugging experience be well documented, eg what command lines are needed, how to specify which jit is being invoked, how to tunnel through the complus settings that are useful when investigating jit issues.

Yeah thats fair. We will work on putting these together.

sandreenko · 2020-11-13T20:28:33Z

SPMI is ready for crossgen2, not sure if we have jitutils support for it to do asm diffs.

However, the debugging experience with crossgen2 was worse than crossgen for me in the past, because:

was not clear how to compile as simple as "crossgen.exe target.dll", instead, you need to know how to create a right ".rsp" file and use it;
passing COMPlus_* variables is harder for debugging, you need to pass them as --codegenopt NgenDump=Repro.Program:Main in the command line and if I remember correctly some were not working (for example Dump).

mangod9 · 2020-11-13T20:37:07Z

@DrewScoggins could you please check the Linux x64 runtime-perf failures, if they are perf related?

erozenfeld · 2020-11-13T21:18:44Z

SPMI is ready for crossgen2, not sure if we have jitutils support for it to do asm diffs.

I added support for asm diffs with crossgen2 a year ago in dotnet/jitutils#228 and dotnet/jitutils#234. It may need to be tweaked if crossgen2 command line args have changed since then. Also, we'll probably need to control crossgen2 multithreading.

kunalspathak · 2020-11-13T22:22:00Z

Curious, how do we see the perf results?

DrewScoggins · 2020-11-13T22:24:09Z

These results are not saved, nor are they even applicable. These tests just run the tests on VMs for a single iteration to ensure that they have not been broken by the change, not to investigate regressions or improvements.

trylek · 2020-11-26T21:47:22Z

The OSX x64 failure is known, #45088.

trylek · 2020-11-26T23:17:05Z

OK, after one retry the PR run is green now. I have triggered the outerloop run and I intend to merge this in tomorrow European morning (around PST midnight) assuming it reasonably passes and no-one objects. I believe this to be the perfect moment - around Thanksgiving the traffic is pretty low so any potential regression will stand out. As discussed previously, I'll send a heads-up e-mail to the runtime repo committers to watch out for any weird behavior after the merge. Thanks everyone for help with various aspects of this change!

mangod9 · 2020-11-26T23:19:40Z

sounds like a good plan. Thanks @trylek

After I deleted the Utf8String-related experimental code from the runtime repo per JanK's suggestion, I can now enable Crossgen2 compilation for System.Private.CoreLib by default. Thanks Tomas

This reverts commit 01c1559872a322469eeceb8bea9084a1d104bc2f.

am11 · 2020-11-27T17:03:36Z

Other than the file size regression, crossgen2 is great! 🙂

# ubuntu x64
$ ./build.sh -c Release
$ du -h artifacts/bin/coreclr/Linux.x64.Release/System.Private.CoreLib.dll

# before
8.9M	artifacts/bin/coreclr/Linux.x64.Release/System.Private.CoreLib.dll
# after
9.6M	artifacts/bin/coreclr/Linux.x64.Release/System.Private.CoreLib.dll

trylek · 2020-11-27T18:37:00Z

Thanks @am11 for pointing that out, I'll take a look. That is surprising as the last time I looked, the CG2 version was actually shorter.

trylek · 2020-11-27T20:01:10Z

OK, so I took a first look using the R2RDump --diff function. I tried this on Windows x64 as there's basically no size difference between the Windows and Linux version (the only actual difference being slightly different calling convention implying subtle changes in some JITted method lengths). The total difference is about 850 KB; almost half of this delta, 380 KB, corresponds to the section DelayLoadMethodCallChunks - in the CG1 version this section only has 33 bytes. @janvorli - am I right to recall it was you who implemented this section? Is it possible that CG1 uses some more compact encoding e.g. by representing all thunks with a single coalesced interval? Supporting this may require putting the thunks into a subsection of its own, implying subtle tweaks to the object writer but the section builder does support this in general, it should be no big deal. I'm looking for the other half that's currently not accounted for by R2RDump for whatever reason.

janvorli · 2020-11-27T20:59:38Z

. @janvorli - am I right to recall it was you who implemented this section?

No, it was @nattress in #39063

…net#44618)" This reverts commit b79e4d3.

)" (#45320) * Revert "Compile System.Private.CoreLib with Crossgen2 by default (#44618)" This reverts commit b79e4d3. * Add a change to ensure libraries are built/tested

trylek added the area-crossgen2-coreclr label Nov 12, 2020

trylek added this to the 6.0.0 milestone Nov 12, 2020

trylek requested review from nattress and janvorli November 12, 2020 23:45

mangod9 approved these changes Nov 13, 2020

View reviewed changes

trylek added a commit to trylek/runtime that referenced this pull request Nov 13, 2020

[NO-MERGE] Combination of dotnet#44624 and dotnet#44618 for lab testi…

a66fd73

…ng purposes

mangod9 self-requested a review November 13, 2020 18:00

mangod9 added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 13, 2020

runfoapp bot mentioned this pull request Nov 13, 2020

BinderTracingTest.ResolutionFlow.cmd timed out #37132

Closed

trylek closed this Nov 19, 2020

trylek reopened this Nov 19, 2020

runfoapp bot mentioned this pull request Nov 19, 2020

Inability to unzip assets during build on Unix x64 #32805

Closed

trylek requested a review from jkotas November 26, 2020 20:24

mangod9 removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 26, 2020

mangod9 approved these changes Nov 26, 2020

View reviewed changes

trylek force-pushed the Crossgen2CorelibByDefault branch from 3d541e5 to 201b00e Compare November 26, 2020 23:32

trylek and others added 3 commits November 27, 2020 09:10

Compile System.Private.CoreLib with Crossgen2 by default

0c75a8f

After I deleted the Utf8String-related experimental code from the runtime repo per JanK's suggestion, I can now enable Crossgen2 compilation for System.Private.CoreLib by default. Thanks Tomas

run perf scenarios

9a069c2

Revert "run perf scenarios"

4f8f6d8

This reverts commit 01c1559872a322469eeceb8bea9084a1d104bc2f.

trylek force-pushed the Crossgen2CorelibByDefault branch from 201b00e to 4f8f6d8 Compare November 27, 2020 08:10

trylek merged commit b79e4d3 into dotnet:master Nov 27, 2020

trylek deleted the Crossgen2CorelibByDefault branch November 27, 2020 15:21

trylek mentioned this pull request Nov 27, 2020

System.Private.CoreLib size regression when compiled with Crossgen2 compared to native Crossgen #45286

Closed

stephentoub added a commit to stephentoub/runtime that referenced this pull request Nov 29, 2020

Revert "Compile System.Private.CoreLib with Crossgen2 by default (dot…

f4ae831

…net#44618)" This reverts commit b79e4d3.

stephentoub mentioned this pull request Nov 29, 2020

Crossgen2-compiled System.Private.CoreLib on Windows x86 causes Null ref in library tests #45317

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile System.Private.CoreLib with Crossgen2 by default #44618

Compile System.Private.CoreLib with Crossgen2 by default #44618

trylek commented Nov 12, 2020

stephentoub commented Nov 12, 2020

trylek commented Nov 12, 2020

stephentoub commented Nov 13, 2020

mangod9 commented Nov 13, 2020

janvorli commented Nov 13, 2020

janvorli commented Nov 13, 2020

trylek commented Nov 13, 2020

trylek commented Nov 13, 2020

AndyAyersMS commented Nov 13, 2020

trylek commented Nov 13, 2020

trylek commented Nov 13, 2020

AndyAyersMS commented Nov 13, 2020

mangod9 commented Nov 13, 2020

MichalStrehovsky commented Nov 13, 2020

DrewScoggins commented Nov 13, 2020

mangod9 commented Nov 13, 2020 •

edited

Loading

AndyAyersMS commented Nov 13, 2020

mangod9 commented Nov 13, 2020

sandreenko commented Nov 13, 2020

mangod9 commented Nov 13, 2020

erozenfeld commented Nov 13, 2020

kunalspathak commented Nov 13, 2020

DrewScoggins commented Nov 13, 2020

trylek commented Nov 26, 2020

trylek commented Nov 26, 2020

mangod9 commented Nov 26, 2020

am11 commented Nov 27, 2020

trylek commented Nov 27, 2020

trylek commented Nov 27, 2020

janvorli commented Nov 27, 2020

Compile System.Private.CoreLib with Crossgen2 by default #44618

Compile System.Private.CoreLib with Crossgen2 by default #44618

Conversation

trylek commented Nov 12, 2020

stephentoub commented Nov 12, 2020

trylek commented Nov 12, 2020

stephentoub commented Nov 13, 2020

mangod9 commented Nov 13, 2020

janvorli commented Nov 13, 2020

janvorli commented Nov 13, 2020

trylek commented Nov 13, 2020

trylek commented Nov 13, 2020

AndyAyersMS commented Nov 13, 2020

trylek commented Nov 13, 2020

trylek commented Nov 13, 2020

AndyAyersMS commented Nov 13, 2020

mangod9 commented Nov 13, 2020

MichalStrehovsky commented Nov 13, 2020

DrewScoggins commented Nov 13, 2020

mangod9 commented Nov 13, 2020 • edited Loading

AndyAyersMS commented Nov 13, 2020

mangod9 commented Nov 13, 2020

sandreenko commented Nov 13, 2020

mangod9 commented Nov 13, 2020

erozenfeld commented Nov 13, 2020

kunalspathak commented Nov 13, 2020

DrewScoggins commented Nov 13, 2020

trylek commented Nov 26, 2020

trylek commented Nov 26, 2020

mangod9 commented Nov 26, 2020

am11 commented Nov 27, 2020

trylek commented Nov 27, 2020

trylek commented Nov 27, 2020

janvorli commented Nov 27, 2020

mangod9 commented Nov 13, 2020 •

edited

Loading