Preparation to introduce parallelism into CrossGen2 #27068
Conversation
- Change dictionaries in ReadyToRunCodegenNodeFactory and ReadyToRunSymbolNodeFactory to NodeCaches (i.e. ConcurrentDictionary, at the moment) - Add structs to act as keys for the above NodeCaches (MethodFixupKey, DynamicHelperKey, ReadyToRunHelperKey) - Synchronize logger - Update some Dictionaries to ConcurrentDictionary
hoyosjs
requested review from
MichalStrehovsky,
trylek,
fadimounir and
sergiy-k
src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs
Outdated
Show resolved
Hide resolved
| @@ -48,7 +51,8 @@ public override int GetHashCode() | |||
| unchecked(Method.GetHashCode() * 31) ^ | |||
| (IsUnboxingStub ? -0x80000000 : 0) ^ | |||
| (IsInstantiatingStub ? 0x40000000 : 0) ^ | |||
| (IsPrecodeImportRequired ? 0x20000000 : 0); | |||
| (IsPrecodeImportRequired ? 0x20000000 : 0) ^ | |||
| (SignatureContext.GetHashCode() * 23); | |||
There was a problem hiding this comment.
SignatureContext [](start = 17, length = 16)
Can this ever be null? cc @trylek to verify if we need to add null checks for this or not
There was a problem hiding this comment.
I believe that the signature context may never be null. The signature context basically represents the token context and as such it gives meaning to the tokens themselves, so to say; without it the signatures are just not meaningful. Please remember our recent discussion related to token context during devirtualization in JIT, especially the large version bubble compilation mode is extremely sensitive to token consistency.
...s/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs
Show resolved
Hide resolved
| if (!_fieldAddressCache.TryGetValue(fieldDesc, out result)) | ||
| public readonly FieldDesc Field; | ||
| public readonly SignatureContext SignatureContext; | ||
|
|
There was a problem hiding this comment.
I think now that we use .Equals and .GetHashCode on SignatureContext objects, we should probably make it an IEquatable class and provide our implementation of these 2 APIs.
@MichalStrehovsky @trylek do you guys agree?
There was a problem hiding this comment.
Yes, seems like SignatureContext is not interned and we're potentially making lots of new ones. We should override Equals and GetHashCode.
I tend to only implement IEquatable if it's a struct (to avoid boxing). It doesn't make much difference for classes.
| @@ -25,6 +24,29 @@ public interface IEETypeNode | |||
| TypeDesc Type { get; } | |||
| } | |||
|
|
|||
| public struct NodeCache<TKey, TValue> | |||
| { | |||
There was a problem hiding this comment.
For the future, it would be good to not do this kind of refactoring (moving blocks of code around while making some changes to them) when the PR contains other numerous changes. It makes it easier to review.
For instance, I see that you have removed the GetOrAdd(TKey key, Func<TKey, TValue> creator) function, but that wasn't easy to spot
.../crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs
Show resolved
Hide resolved
.../crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs
Outdated
Show resolved
Hide resolved
.../crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs
Show resolved
Hide resolved
| @@ -83,5 +83,22 @@ public ModuleToken GetModuleTokenForField(FieldDesc field, bool throwIfNotFound | |||
| { | |||
| return Resolver.GetModuleTokenForField(field, throwIfNotFound); | |||
| } | |||
|
|
|||
| public bool Equals(SignatureContext other) | |||
There was a problem hiding this comment.
Nit: there shouldn't be a need for this, and we can just have the Equals(object) API. The other key structs have it because they implement IEquatable, because they are all structs and the IEquatable is the way to avoid boxing/unboxing, but SignatureContext is a class.
It's fine to leave this the way it is. This is just a FYI comment
|
LGTM. @trylek I just want to confirm with you that there is no way the SignatureContext can ever be null before merging? |
After change dotnet/coreclr#27068 we are creating multiple instances of CorInfoImpl. That broke the scenario when jitpath is set: we are calling NativeLibrary.SetDllImportResolver more than once, which results in `System.InvalidOperationException: A resolver is already set for the assembly.` This fix ensures that we call NativeLibrary.SetDllImportResolver at most once. This change also ensures that we set the resolver before attempting to load the jit. That fixes the --jitpath scenario on Linux.
This change does not introduce any parallelism, that (and tests for determinism) will come in the next PR.