[r2r] Enable generic cycles detection by default#128957
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates crossgen2 ReadyToRun (R2R) compilation to enable generic-cycle detection by default and to proactively avoid GVM dependency expansion for methods/types that are flagged as potentially participating in generic cycles. The goal is to prevent explosive graph growth during dependency analysis that can lead to compiler overflows/timeouts (as seen in R2R compilation of System.Linq.Parallel.Tests).
Changes:
- Enable generic-cycle detection by default for R2R by setting the builder’s generic cycle cutoffs to
ReadyToRunCompilerContextdefaults. - Extend the generic cycle detection infrastructure with a “could be in a cycle” query and plumb it through the R2R
NodeFactory(CanBeInGenericCycle). - Make
NodeFactory.GVMDependencies(...)returnnullfor potentially-cyclic entities and update GVM dependency consumers to tolerate and skip these dependencies; remove the older GVM-node-local generic cycle check.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs | Adds a CanBeInCycle query on GenericCycleDetector for fast cycle-membership checks. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/GVMDependenciesNode.cs | Updates dynamic dependency discovery to handle GVMDependencies(...) returning null; removes per-node generic-cycle try/catch in R2R path. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs | Enables generic-cycle detection by default by initializing cutoffs to ReadyToRunCompilerContext defaults. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Adds CanBeInGenericCycle and skips creating GVM dependency nodes for potentially-cyclic definitions (returns null). |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs | Avoids adding a virtual dispatch GVM dependency when GVMDependencies(...) returns null. |
| { | ||
| list = list ?? new DependencyList(); | ||
| list.Add(factory.GVMDependencies(Method), "Virtual dispatch dependency"); | ||
| GVMDependenciesNode gvmNode = factory.GVMDependencies(Method); |
There was a problem hiding this comment.
We just checked for the cycle on line 68 above, is this necessary?
In general I agree with copilot that NodeFactory returning null is very nonstandard.
We treat generic cycle same as "TypeRef/MemberRef doesn't resolve" or similar exceptional situations. It's typically an exception that is caught by a catch (TypeSystemException) catchall (that is just "there's a problem with the input and we are at a spot where we can just shrug and move on").
There was a problem hiding this comment.
Extracted the new cycle check as a separate method on the NodeFactory so that it is the callers responsability to check if it actually needs a GVMDependenciesNode.
I'm not sure why the check on line 68 would be relevant. That is a different code path, handling the case where the method is not generic, so these blocks are exclusive for a method. Also these cycle checks are not identical. The standard DetectCycle checks first if the generic type arguments for an entity can be involved in generic cycles and, if so, it checks if the instantiated entity exceeds some treshold of nested instantiations. So the method: Method<Gen<Gen<Gen<int>>>> could in theory be compiled because it doesn't recurse deep enough (yet), even though its generic type argument could be involved in generic cycles. The new check that I'm adding prohibits an instantiation of Method<T> of ever taking part in GVM expansion, because, as the System.Linq.Parallel.Tests assembly showed, it could still lead to aggressive exponential expansion, before we are even able to put a stop to it with the standard DetectCycle.
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Following the addition of GVM handling in r2r via dynamic dependencies, System.Linq.Parallel.Tests fails to compile, by overflow due to excessive compilation. Enabling the scanning for generic cycles alone doesn't fix this (--enable-generic-cycle-detection). This testsuite also fails to compile on NativeAot where it is disabled. It seems like, despite having generic cycle checks inside the GVM expansion, the amount of nodes still grows at an excessive rate, breaking the compilation. As a fix, we ensure that we have the `_genericCycleDetector` initialized by default on r2r and use it to query for types/methods that could be part of generic cycles (from the computed `ModuleCycleInfo` information). We conservatively avoid any GVM scan for these entities, to avoid excessive compilation and potential size increases, since we have the flexibility to fallback to jit/interpreter at runtime. As a side effect, this enables the generic cycle checks for other areas of R2R, but, given we will start compiling more methods with generics, it might be a good thing to have it enabled anyway.
BrzVlad
force-pushed
the
fix-r2r-gvm-cycles
branch
from
4bd014c to
8875f9e
Compare
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if (genericTypeArguments.Length != genericTypeParameters.Length) | ||
| return; // Alternatively throw TypeLoadException |
| public bool CanBeInGenericCycle(MethodDesc method) | ||
| { | ||
| if (_genericCycleDetector is null) | ||
| return false; | ||
|
|
||
| MethodDesc methodDefinition = method.GetTypicalMethodDefinition(); | ||
| return _genericCycleDetector.CanBeInCycle(methodDefinition) | ||
| || _genericCycleDetector.CanBeInCycle(methodDefinition.OwningType); | ||
| } |
Following the addition of GVM handling in r2r via dynamic dependencies, System.Linq.Parallel.Tests fails to compile, by overflow due to excessive compilation. Enabling the scanning for generic cycles alone doesn't fix this (--enable-generic-cycle-detection). This testsuite also fails to compile on NativeAot where it is disabled. It seems like, despite having generic cycle checks inside the GVM expansion, the amount of nodes still grows at an excessive rate, breaking the compilation.
As a fix, we ensure that we have the
_genericCycleDetectorinitialized by default on r2r and use it to query for types/methods that could be part of generic cycles (from the computedModuleCycleInfoinformation). We conservatively avoid any GVM scan for these entities, to avoid excessive compilation and potential size increases, since we have the flexibility to fallback to jit/interpreter at runtime.As a side effect, this enables the generic cycle checks for other areas of R2R, but, given we will start compiling more methods with generics, it might be a good thing to have it enabled anyway.
Also removes the previous generic cycles check from GVMDependenciesNode which no longer seem necessary.
Fixes #128694