Refactor compilation module group by davidwrighton · Pull Request #1889 · dotnet/runtime

davidwrighton · 2020-01-17T23:01:14Z

Our CompilationModule groups used for single method compilation and general compilation had diverged substantially, reducing the effectiveness of single method compilation substantially
The profiler guided optimization logic also needs to use the VersionWithCode logic to identify which methods are interesting to consider, this will happen in a future change
The VersionsWith logic in CompilationModuleGroup did not correctly model versionable logic.
- Methods with generic parameters were not handled correctly. Method generic parameters did not contribute to the VersionsWith logic.
- The newer ideas in our recent work on upcoming enhancements to crossgen2 driven compilation enhancements are aligned with this effort
- Add support for the concept of considering instantiations over primitive types to be legal even if the primitive types aren't part of the version bubble. (Only if there are no constraints in the generic)

…crossgen2 - We now have 2 difference compilation module groups (the single assembly and single method groups) - These groups have grown differences over time that reduce the value of the single method group - Refactor them to share the single assembly algorithms for most stuff - Also, fixup the VersionsWith logic to match the desired behavior

… code duplication and enhance correctness - Our CompilationModule groups used for single method compilation and general compilation had diverged substantially, reducing the effectiveness of single method compilation substantially - The profiler guided optimization logic also needs to use the VersionWithCode logic to identify which methods are interesting - The VersionsWithCode logic in CompilationModuleGroup did not correctly model versionable logic. - Methods with generic parameters were not handled correctly - The newer ideas in our recent work on upcoming enhancements to crossgen2 driven compilation enhancements are aligned with this effort

…ve a constrained call to a ToString method not located within the version bubble

davidwrighton · 2020-01-18T00:48:39Z

@dotnet/crossgen-contrib Please be aware of this

trylek

Looks great overall, apart from the code changes I really appreciate your diligence in producing the design / guidance doc as part of the change.

trylek · 2020-01-18T01:01:05Z

+The second approach is to split compilation up into assembly level units, run the heuristics per assembly, generate the completely local generics in the individual assemblies, and then nominate a final mop up assembly that consumes a series of data files produced by the individual assembly compilations and holds all of the stuff that didn't make sense in the individual assemblies. In my opinion this second approach would be better for debug builds, but the first approach is strictly better for release builds, and really shouldn't be terribly slow.
+
+# Proposal
+Address the reducing generics duplication concern by implementing composite R2R files instead of attempting to build a multifile approach, where each layer is expected to know the exact layer above, or be fully R2R with respect to all layers above. Loading of R2R images built with version bubble dependence will lazily verify that version rules are not violated, and produce a FailFast if version bubble usage rules are used incorrectly. For customers which produce customized versions of the underlying frameworks, if any differences are present they will likely be forced to provide their own targeting packs to use the fully AOT layered scenarios.


I concur with Michal that we should have at least some way of identifying such a mismatch early - according to JanK R2R is conceptually just a code cache, not something you expect to tear your app down due to weird dependencies.

I think that a GitHub PR where this is an ancillary document to the main thrust of the PR is not the right place to discuss this. Please either open an issue to discuss this, or once this is checked in, make a PR to adjust this proposal statement.

MichalStrehovsky · 2020-01-20T11:05:04Z

            Utf8StringBuilder sb = new Utf8StringBuilder();
+            sb.Append($@"MethodWithGCInfo(");
            AppendMangledName(factory.NameMangler, sb);
+            sb.Append($@")");


I wanted to nitpick about the useless $@ prefix, but I see this is consistent with useless $@ prefixes in a bunch of other places so I'll just cringe silently instead.

This line has a nice combo of characters one needs to hold Shift for and the readability is competitive with Perl.

Lol, you don't need to cringe silently. This is ridiculous, and an easy fix which I've done, although I'd say the readability isn't quite that bad. :)

MichalStrehovsky · 2020-01-20T11:07:01Z

+{
+    public abstract class ReadyToRunCompilationModuleGroupBase : CompilationModuleGroup
+    {
+        protected HashSet<ModuleDesc> _compilationModuleSet;


Maybe we can mark these readonly?

MichalStrehovsky · 2020-01-20T11:20:41Z

+            else
+                return false;
+
+            if (method == method.GetTypicalMethodDefinition())


Should this be GetMethodDefinition? It doesn't seem like we need to take ComputeInstantiationVersionsWithCode below for non-generic methods on generic types.

Yes that seems right. Functionally its the same, but calling the extra method is unnecessary.

MichalStrehovsky · 2020-01-20T11:23:41Z

+
+        private bool ComputeTypeVersionsWithCode(TypeDesc type)
+        {
+            if (type == type.Context.CanonType)


Should we use IsCanonicalDefinitionType to also cover other types of canon we might add in the future (universal, enum integer, etc.)?

Yes. This is a holdover from some debugging I was doing that led me to discover we were loading System.__Canon as an EcmaType as well as a CanonType.

MichalStrehovsky · 2020-01-20T11:25:35Z

+                        }
+
+                        GenericParameterDesc genericParam = (GenericParameterDesc)entityDefinitionInstantiation[iInstantiation];
+                        if (genericParam.Constraints.HasFlag(GenericConstraints.ReferenceTypeConstraint))


This is shorter and avoids boxing:

Suggested change

if (genericParam.Constraints.HasFlag(GenericConstraints.ReferenceTypeConstraint))

if (genericParam.HasReferenceTypeConstraint)

MichalStrehovsky · 2020-01-20T11:28:35Z

+            }
+            return true;
+
+            bool ComputeInstantiationTypeVersionsWithCode(TypeDesc type)


Nit: I tend to prefix local methods with static to avoid any perf gotchas caused by the C# compiler allocating a closure and just don't use the language feature if static doesn't cut it.

MichalStrehovsky · 2020-01-20T11:38:39Z

        }

-        public override bool GeneratesPInvoke(MethodDesc method)
+        public sealed override void ApplyProfilerGuidedCompilationRestriction(ProfileDataManager profileGuidedCompileRestriction)


I would rather see this as a constructor argument because immutable classes tend to be better in multithreaded codebases - or can you at least make this fail if _profileGuidedCompileRestriction was already set?

I'll make it fail. The reason it isn't a constructor argument, is that the profiler guided infrastructure is going to need to use the CompilationModuleGroup, and it gets ugly fast. However, making the function throw if set is completely a great idea.

I'll also make it fail if we try to run down the code that uses this thing without setting it up. It would be bad to use a null profieldata manager and then a real one.

davidwrighton · 2020-01-22T22:25:50Z

@fadimounir I found that the crossgen2determinism tests were failing with this change due to an issue with ModuleToken resolution. Could you take a look at my fix for that?

fadimounir · 2020-01-22T22:42:30Z

I found that the crossgen2determinism tests were failing with this change

Did we end up having multiple token values for the same TypeSystemEntity? I'm curious how was determinism broken?
The fix LGTM

fadimounir · 2020-01-22T22:53:40Z

+                            return false;
+
+                        // This checks to see if the type constraints list is empty
+                        if (genericParam.TypeConstraints.GetEnumerator().MoveNext())


Does this enumerate a constraint like IEquatable? If so, shouldn't we detect if SPC is in the version bubble before returning false (also what the comment at line 244 says)

We did that already. That is what line 239+241 did. We only hit this case when compiling without corelib in the bubble.

davidwrighton · 2020-01-23T00:15:23Z

@fadimounir Yes, we had multiple different token values, as the way this ended up resolving is that there were multiple tokens that pointed at MyGen<>.GcValue. (One of them was to MyGen<!!1>.GcValue, and the other was MyGen<string>.GcValue) Now, when we were generating the token FieldFixupSignature for MyGen<object>.GcValue, we need to use a MemberRef token, and we only need the Name+Sig portion of the memberref to be correct as we specify ENCODE_FIELD_SIG_OwnerType. In that case, we can choose to use either of the memberrefs, and the test managed to get unlucky enough to generate two different images reliably.

davidwrighton added 3 commits January 17, 2020 11:44

- Add test to make sure we're covering the new case where we might ha…

a169fd4

…ve a constrained call to a ToString method not located within the version bubble

dotnet deleted a comment from azure-pipelines Bot Jan 18, 2020

davidwrighton added the area-ReadyToRun label Jan 18, 2020

davidwrighton requested review from MichalStrehovsky, nattress and trylek and removed request for nattress January 18, 2020 00:49

trylek approved these changes Jan 18, 2020

View reviewed changes

jkotas reviewed Jan 18, 2020

View reviewed changes

Comment thread docs/design/coreclr/botr/crossgen2-compilation-structure-enhancements.md Resolved

MichalStrehovsky reviewed Jan 20, 2020

View reviewed changes

davidwrighton added 2 commits January 21, 2020 17:46

Apply feedback

0095e2e

Fix determinism issue in FieldRef/TypeRef to ModuleToken resolution

948524b

fadimounir reviewed Jan 22, 2020

View reviewed changes

fadimounir approved these changes Jan 23, 2020

View reviewed changes

davidwrighton merged commit fd7a5e8 into dotnet:master Jan 23, 2020

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

davidwrighton deleted the refactor_compilation_module_group branch April 20, 2021 17:39

	if (genericParam.Constraints.HasFlag(GenericConstraints.ReferenceTypeConstraint))
	if (genericParam.HasReferenceTypeConstraint)

Conversation

davidwrighton commented Jan 17, 2020

Uh oh!

davidwrighton commented Jan 18, 2020

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented Jan 22, 2020

Uh oh!

fadimounir commented Jan 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidwrighton commented Jan 23, 2020 •

edited

Loading