Detect generic recursion in generic lookups

[MichalStrehovsky]

We currently cut off generic recursion at two spots:

1. When there's a direct call to something that causes a recursion (e.g. Method<T> calls Method<Gen<T>> and Gen is a struct).
2. When there's recursion in the generic dictionary (e.g. the sample above but Gen is a class).

There's yet another variation of this - indirect call to something that causes a recursion. Above two won't capture this because indirect calls don't go through the codepath 1, and codepath 2 is already too late (the recursion happens within the canonical code we already generated and invalidating dictionary entries is too late).

This adds one more spot to cut things off.

This is hit in Npgsql, but only on Linux, for whatever reason.

[hez2010]

Thanks. After this PR all tests in #776 will pass if I reduce the generic recursion depth to 4.
BTW is there any way to specify the maximum depth of generic recursion?

[MichalStrehovsky]

Thanks. After this PR all tests in #776 will pass if I reduce the generic recursion depth to 4.

Awesome, thanks for checking on this! Let's keep the issue open until there's a good feedback experience around this - I would like to avoid the spurious NullReferenceExceptions when cutoff happens, and I would like the compiler to print some sort of useful feedback when the cutoff happens so that people can ideally remove the generic cycle from the source.

BTW is there any way to specify the maximum depth of generic recursion?

It's currently hardcoded to 4:

runtimelab/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/LazyGenerics/ModuleCycleInfo.cs

Lines 34 to 42 in 748d4b3

	// Chosen rather arbitrarily. For the app that I was looking at, cutoff point of 7 compiled
	// more than 10 minutes on a release build of the compiler, and I lost patience.
	// Cutoff point of 5 produced an 1.7 GB object file.
	// Cutoff point of 4 produced an 830 MB object file.
	// Cutoff point of 3 produced an 470 MB object file.
	// We want this to be high enough so that it doesn't cut off too early. But also not too
	// high because things that are recursive often end up expanding laterally as well
	// through various other generic code the deep code calls into.
	private const int CutoffPoint = 4;

It would be nice to have it configurable. If you're interested in contributing that, I think we can pipe that through by:

• Move the IsDeep* methods and the constant to the GenericCycleDetector class below
• Add a constructor to GenericCycleDetector that takes the cutoff point value.
• The GenericCycleDetector is currently instantiated here so move it to the CompilerTypeSystemContext constructor. Add the cutoff parameter to the CompilerTypeSystemContext constructor as a new parameter.
• Add a new parameter to the driver and use that to create the CompilerTypeSystemContext. Set the default to 4 as it was.
• It might be nice to make it so that value 0 completely disables all of this.

[hez2010]

BTW, how about allowing users to specify the cutoff depth per type/method in rd.xml, while also providing an option for setting default global cutoff depth?

<Type Name="Foo`1[[!!0]]" MaxGenericDepth="4" />
<Type Name="Foo">
    <Method Name="Bar`1[[System.Int32,System.Private.CoreLib], [!!0]]" MaxGenericDepth="8" />
</Type>

[MichalStrehovsky]

Do you have a motivating example where we would need this to be controllable by type/method?

Having just one generic recursion in the program is already bad enough. Having two different recursions that also need different cutoff points sounds like a nightmare scenario.