Crossgen2 determinism fixes #164

SrivastavaAnubhav · 2019-11-21T00:20:55Z

NativeFormatWriter's HashTable save now uses OrderBy, so the output deterministic if the values are entered deterministically
Lock GetILBytes in EcmaMethodIL so we don't accidentally get two different addresses for the same code
Revert ManifestMetadataTableNode change to an older version: we don't add the input module's assemblies to manifestAssemblies in the constructor (we add to assemblyRefToIdMap) so some things were failing. The dictionary might also be faster than the List for lookup.
Do not flush the ILCache before compiling all methods in a batch. This might have caused the same MethodDesc to have two different MethodIL objects.

SrivastavaAnubhav · 2019-11-21T00:51:55Z

@dotnet/crossgen-contrib

src/coreclr/src/tools/crossgen2/Common/Internal/NativeFormat/NativeFormatWriter.cs

fadimounir · 2019-11-21T01:02:01Z

src/coreclr/src/tools/crossgen2/Common/TypeSystem/IL/EcmaMethodIL.cs

-            return (_ilBytes = ilBytes);
+            // This lock is necessary since the JIT requires that we provide the same address
+            // for the IL buffer each time (it uses the address to detect recursive calls).
+            // Since MethodBodyBlock.GetAllBytes creates a new array each time, if two threads


Nit: Just mention the name of the API we're calling here (GetILBytes) to reduce confusion. There's no GetAllBytes here.

Consider using Lazy to avoid writing our own double-checked locking?

I like what Jan suggested here, to use Interlocked.CompareExchange

fadimounir · 2019-11-21T01:08:50Z

...n2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs

-
-            if (!_manifestAssemblies.Contains(assemblyName))
+            int assemblyRefIndex;
+            if (!_assemblyRefToModuleIdMap.TryGetValue(assemblyName.Name, out assemblyRefIndex))


This is wrong: this will cause missing entries in the _manifestAssemblies (the bug that I fixed recently).

The right way to fix this is to get rid of _manifestAssemblies, and emit a sorted version of _assemblyRefToModuleIdMap in GetData(), sorted by module ID

And make _assemblyRefToModuleIdMap a Dictionary<AssemblyName, int>

fadimounir · 2019-11-21T01:13:55Z

...sgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs

-            EcmaModule targetModule = _signatureContext.GetTargetModule(_arrayType);
-            SignatureContext innerContext = dataBuilder.EmitFixup(r2rFactory, ReadyToRunFixupKind.READYTORUN_FIXUP_NewArray, targetModule, _signatureContext);
-            dataBuilder.EmitTypeSignature(_arrayType, innerContext);
+            if (!relocsOnly)


Nit: would be nice to follow the same convention we have in other places:

if (relocsOnly) { return new ObjectData(Array.Empty<byte>(), null, 1, null); }

It looks like we use a mix of both, but I like the if (relocsOnly) one better because of the indentation. Can I move all of them over to that one?

I mean the ones which return trivially like that. There are some legitimate uses of it as well.

In that case, leave it the way it is.

fadimounir

Generally looks good, but needs a few fixes.

As @davidwrighton mentioned, you will need to think about how to add tests for this determinism work. That's the kind of stuff that is very easy for other folks to break if there's no test to catch it.

fadimounir · 2019-11-21T01:28:05Z

if there's no test to catch it.

A good place to start is to add a test that compiles all managed assemblies in coreroot twice with different seeds, and diffs the output.

jkotas · 2019-11-21T02:18:32Z

src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs

-            if (_methodILCache.Count > 1000)
-                _methodILCache = new ILCache(_methodILCache.ILProvider, NodeFactory.CompilationModuleGroup);
-
+            // TODO: Remove the ILCache and use the pinned heap for the IL stream once this GC


I do not see why we would ever want to remove the ILCache. Using pinned heap is not going to help. I would delete this comment.

why we would ever want to remove the ILCache

I don't see why we need it in the first place. This just seems to help releasing the pinning of the IL byte arrays to reduce GC fragmentation. The IL streams are saved on the type system method object, and there's no point in "caching".

My understanding is that the pinned heap will not have fragmentation. If so, we should just allocate the IL byte arrays on it, and remove the ILCache

ILCache does not do any pinning. All pinning is done by jitinterface (CorInfoImpl.cs).

The purpose of ILCache is to cache the ILCode for given MethodDesc. The algorithm to compute ILCode for given MethodDesc is not cheap.

My understanding is that the pinned heap will not have fragmentation.

Pinned heap will have tend to have worse fragmentation than regular heaps because of it cannot be compacted.

Ok, I see what you mean. I debugged this, and it's more involved than I thought. I was under the impression that GetMethodIL() would directly just go to the EcmaMethodDesc's IL byte array that we load from metadata. This is why I was under the impression that the ILCache was used to free up the pins.

@SrivastavaAnubhav please delete the comment then.

jkotas · 2019-11-21T02:22:54Z

src/coreclr/src/tools/crossgen2/Common/TypeSystem/IL/EcmaMethodIL.cs

+            // call this on the same function then it is possible for both to set _ilBytes.
+            // Then, one of the threads will receive the first value on the first call and the
+            // second value on all subsequent calls, and we will not detect the recursive call.
+            lock (this)


This should rather use Interlocked.CompareExchange

src/coreclr/src/tools/crossgen2/Common/TypeSystem/IL/EcmaMethodIL.cs

…areExchange. Remove comment saying that ILCache would eventually be removed. Fix ModuleToIndex not adding all assemblies.

fadimounir · 2019-11-23T06:41:00Z

src/coreclr/src/tools/crossgen2/Common/TypeSystem/IL/EcmaMethodIL.cs

-                byte[] ilBytes = _methodBody.GetILBytes();
-                return (_ilBytes = ilBytes);
-            }
+            Interlocked.CompareExchange(ref _ilBytes, _methodBody.GetILBytes(), null);


I think this needs to be in a loop to handle race conditions

MSDN has a sample here: https://docs.microsoft.com/en-us/dotnet/api/system.threading.interlocked.compareexchange?view=netframework-4.8

This is correct as is. No loop needed.

Ah, right. Sorry, my bad. I got confused by the so many other compare-exchanges we have in the runtime code base :). Thanks for catching that Jan!

fadimounir · 2019-11-23T06:55:11Z

...n2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ManifestMetadataTableNode.cs

@@ -163,7 +176,8 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
               fieldList: MetadataTokens.FieldDefinitionHandle(1),
               methodList: MetadataTokens.MethodDefinitionHandle(1));

-            foreach (AssemblyName assemblyName in _manifestAssemblies)
+            IEnumerable<AssemblyName> manifestAssemblies = _moduleIdToAssemblyNameMap.OrderBy(x => x.Key).Select(x => x.Value);


Nit: It might look nicer to write it without .Select(), like:

foreach (var index in _moduleIdToAssemblyNameMap.Keys.OrderBy(...)) { use _moduleIdToAssemblyNameMap[index] }

fadimounir · 2019-11-24T06:18:39Z

The changes look good to me and ready to merge (modulo the merge conflicts). The only remaining thing so far is adding some tests to catch regressions.

SrivastavaAnubhav · 2019-11-26T18:12:43Z

My fork was messed up by the private -> public switch of this repo, so I've remade the fork and the PR here: #306

More determinism fixes.

6f71413

Dotnet-GitSync-Bot added the area-crossgen2-coreclr label Nov 21, 2019

fadimounir reviewed Nov 21, 2019

View reviewed changes

src/coreclr/src/tools/crossgen2/Common/Internal/NativeFormat/NativeFormatWriter.cs Show resolved Hide resolved

fadimounir reviewed Nov 21, 2019

View reviewed changes

fadimounir suggested changes Nov 21, 2019

View reviewed changes

jkotas reviewed Nov 21, 2019

View reviewed changes

src/coreclr/src/tools/crossgen2/Common/TypeSystem/IL/EcmaMethodIL.cs Outdated Show resolved Hide resolved

Address PR comments. Replace lock in GetILBytes with Interlocked.Comp…

d8c3522

…areExchange. Remove comment saying that ILCache would eventually be removed. Fix ModuleToIndex not adding all assemblies.

fadimounir reviewed Nov 23, 2019

View reviewed changes

SrivastavaAnubhav closed this Nov 26, 2019

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

dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Mar 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crossgen2 determinism fixes #164

Crossgen2 determinism fixes #164

SrivastavaAnubhav commented Nov 21, 2019

SrivastavaAnubhav commented Nov 21, 2019

fadimounir Nov 21, 2019

cshung Nov 21, 2019

fadimounir Nov 21, 2019 •

edited

Loading

fadimounir Nov 21, 2019

fadimounir Nov 21, 2019

fadimounir Nov 21, 2019

SrivastavaAnubhav Nov 21, 2019

SrivastavaAnubhav Nov 21, 2019

fadimounir Nov 21, 2019

fadimounir left a comment

fadimounir commented Nov 21, 2019

jkotas Nov 21, 2019

fadimounir Nov 21, 2019

jkotas Nov 21, 2019

fadimounir Nov 22, 2019

jkotas Nov 21, 2019

fadimounir Nov 23, 2019

fadimounir Nov 23, 2019

jkotas Nov 23, 2019

fadimounir Nov 24, 2019

fadimounir Nov 23, 2019

fadimounir commented Nov 24, 2019

SrivastavaAnubhav commented Nov 26, 2019

Crossgen2 determinism fixes #164

Crossgen2 determinism fixes #164

Conversation

SrivastavaAnubhav commented Nov 21, 2019

SrivastavaAnubhav commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fadimounir Nov 21, 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

fadimounir left a comment

Choose a reason for hiding this comment

fadimounir commented Nov 21, 2019

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 Nov 24, 2019

SrivastavaAnubhav commented Nov 26, 2019

fadimounir Nov 21, 2019 •

edited

Loading