JIT/jit64/opt/rngchk/RngchkStress2.cs failing to build with error CS8078: An expression is too long or complex to compile

[radical]

Failing on osx-x64 Release AllSubsets_Mono_Minijit_RuntimeTests minijit:

Failing build:

/Users/runner/work/1/s/src/tests/JIT/jit64/opt/rngchk/RngchkStress2.cs(527,796): error CS8078: An expression is too long or complex to compile [/Users/runner/work/1/s/src/tests/JIT/jit64/opt/rngchk/RngchkStress2_o.csproj] [/Users/runner/work/1/s/src/tests/build.proj]
##[error]src/tests/JIT/jit64/opt/rngchk/RngchkStress2.cs(527,796): error CS8078: An expression is too long or complex to compile [/Users/runner/work/1/s/src/tests/JIT/jit64/opt/rngchk/RngchkStress2_o.csproj]

This was hit on #87522 .

{
  "ErrorMessage": "An expression is too long or complex to compile",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=315357
Result validation: ✅ Known issue matched with the provided build.

Report

Build	Definition	Step Name	Console log	Pull Request
533692	dotnet/runtime	Build Tests	Log	#95758
532325	dotnet/runtime	Build Tests	Log	#95758
531205	dotnet/runtime	Build Tests	Log	#95758
528402	dotnet/runtime	Build Tests	Log

Summary

24-Hour Hit Count	7-Day Hit Count	1-Month Count
0	0	4

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Failing on osx-x64 Release AllSubsets_Mono_Minijit_RuntimeTests minijit:

Failing build:

/Users/runner/work/1/s/src/tests/JIT/jit64/opt/rngchk/RngchkStress2.cs(527,796): error CS8078: An expression is too long or complex to compile [/Users/runner/work/1/s/src/tests/JIT/jit64/opt/rngchk/RngchkStress2_o.csproj] [/Users/runner/work/1/s/src/tests/build.proj]
##[error]src/tests/JIT/jit64/opt/rngchk/RngchkStress2.cs(527,796): error CS8078: An expression is too long or complex to compile [/Users/runner/work/1/s/src/tests/JIT/jit64/opt/rngchk/RngchkStress2_o.csproj]

This was hit on #87522 .

Author:	radical
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[radical]

log - full stack trace: https://gist.github.com/radical/8ba41143e077b8b6eab371237c1756aa

[JulieLeeMSFT]

@jcouv, thanks for quickly disabling the test. Are you looking into the rootcause of the failure?

[radical]

@jcouv My attempts at disabling the tests didn't quite work. Could you please take a look?

[jcouv]

Thanks for the ping. I'm taking a look. I wasn't able to repro yet.

[jcouv]

@radical I'm trying to repro this issue, but am unable so far.
Basically, I copied the deeply nested code from RngchkStress2.cs and put in a compiler unittest. Then I call the API (SemanticModel.GetOperation) reported in the overflow stack trace. (btw, thanks for that stack trace)
I'm able to do that in debug mode, which normally consumes a lot more stack. But I get no overflow.

Looking at the code involved in the stack trace (CSharpOperationFactory, which is a big visitor), I see that code has not changed since January, so it feels unlikely that we've increased the stack consumption there.

Some thoughts so far:

• it's possible that the tree that we feed to CSharpOperationFactory has changed, but that also feels unlikely (I looked at the history of "Operation" tests, but saw nothing significant)
• maybe the compiler used is not a recent version that I'm looking at. Doing the repro with an extra #error version in the problematic file would help confirm the version that is used (it will be output as an error message)
• it doesn't look like the toolset compiler is being updated as part of the problematic PR. So I'm thinking that root cause may lie elsewhere. Getting version from #error version before the change would help confirm that (I expect it will be the same as after the change)
• I wonder whether is there something special about the machine or the environment that makes the stack size more constrained?
• the other factor that affects the stack usage is the runtime, which was updated as part of this PR. Would it be possible to capture a dump for this crash?

[jkotas]

This was hit on #87522 .

This PR upgraded the .NET SDK used to build the repo to .NET 8 Preview 5.

This intermittent build error is likely caused by tiered PGO instrumentation that was enabled in .NET 8 Preview 5 by default. The PGO instrumentation may cause program to use different amount of stack space from run to run.

[jcouv]

Thanks @jkotas, that makes sense. Unassigned myself and marked the issue as "untriaged" so that it can be assigned to a new owner.

[am11]

cc @jakobbotsch (I think this will go away with new PGO collection/update)

[jakobbotsch]

I think we should just simplify the test if it is expected that Roslyn will rely on stack for deeply nested input programs. Note that the failures are on osx-x64 which, IIRC, have threads with notoriously small stack limits.

It is certainly expected from our side that new JIT changes may cause generated native code to use more stack. In this case it seems realistic that it is related to tiered PGO being enabled in preview 5. I would guess the source is new inlining as a result of having PGO data available.

[jakobbotsch]

FWIW, the test has 193 nested for loops in it, and the stack trace posted above contains CreateBoundForStatementOperation 179 times. So let's just remove 50 levels of nesting.

[jaredpar]

Roslyn is a bit special in this regard since it has unbounded recursion that is dependent on user provided input.

Guessing Roslyn is not alone here.

[jakobbotsch]

Guessing Roslyn is not alone here.

Agreed, but I would expect this to be a much smaller class of applications than applications that use the thread pool. In any case the to idea make the stack limits consistent across platforms seems good to me.

I would like to understand what exactly has changed since we are seeing this while building a number of different tests, even some innocent looking ones. I hit this while building https://github.com/dotnet/runtime/blob/261e3a40cd9faa240da82aacd8d821469703e339/src/tests/JIT/Methodical/VT/etc/ctor_recurse.cs locally today, and I have seen it on https://github.com/dotnet/runtime/blob/1aded3f65f6fd13a60eb43c191f4ea8aa27bf5dc/src/tests/JIT/Regression/JitBlue/Runtime_85602/Runtime_85602.cs in CI recently as well. These do not look super complex, so it's a weird coincidence if nothing major has changed.

[jaredpar]

Historically we've seen this due to a combination of factors:

1. The runtime and roslyn have a number of code samples that run right up against the limits.
2. The runtime makes changes, like improve inlining, that change stack size

Taken together this can lead to the compiler blowing its stack where it previously did not. This typically plays out in a given .NET release by the following sequence of events:

Runtime makes change that improves inlining. That change gets merged into a .NET SDK. The darc PR to merge that SDK into dotnet/roslyn gets blocked because it breaks the EndToEnd tests we have that verify our nesting limits.

Pretty sure we've seen this play out in runtime before but it's been a while.

These do not look super complex, so it's a weird coincidence if nothing major has changed.

I agree that neither of those look very complex. Those hitting the limits would, in my mind, be cause for investigation as to what changed.

FWIW the compiler actually got significantly better in .NET 8 in terms of level of nesting we can handle. In most cases it improved by 4X compared to .NET 7. So this should actually be going the other direction. Always possible you hit a nesting path we don't test though.

[EgorBo]

Minimal repro (it blows up even on Windows after certain iteration - looks like Tier0 codegen is fine):

Prerequsites:

• Remove xunit from RngchkStress2.cs
• Add <PackageReference Include="Microsoft.CodeAnalysis" Version="4.7.0-2.final" />

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Emit;

class Program
{
    const string repo = @"C:/prj/runtime";
    const string config = "windows.x64.Release";
    const string coreRoot = repo + $@"/artifacts/tests/coreclr/{config}/Tests/Core_Root/";
    const string sourceFilePath = repo + @"/src/tests/JIT/jit64/opt/rngchk/RngchkStress2.cs";

    static async Task Main()
    {
        string sourceCode = File.ReadAllText(sourceFilePath);
        for (int i = 0; i < 100; i++)
        {
            const int stackSize = 384 * 1024;
            var thread = new Thread(() => CompileToDll(sourceCode, "Output.dll"), stackSize);
            thread.Start();
            await Task.Delay(100);
            thread.Join();
        }
        Console.WriteLine("Started compilation on background thread...");
        Console.ReadLine();
    }

    private static void CompileToDll(string sourceCode, string outputDllPath)
    {
        SyntaxTree syntaxTree = CSharpSyntaxTree.ParseText(sourceCode);
        string assemblyName = Path.GetRandomFileName();
        CSharpCompilation compilation = CSharpCompilation.Create(
            assemblyName,
            syntaxTrees: new[] { syntaxTree },
            references: new[] {
                MetadataReference.CreateFromFile(coreRoot + "System.Private.CoreLib.dll"),
                MetadataReference.CreateFromFile(coreRoot + "System.Runtime.dll"),
                MetadataReference.CreateFromFile(coreRoot + "System.Console.dll")
            },
            options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));
        EmitResult result = compilation.Emit(outputDllPath);
        if (result.Success)
        {
            Console.WriteLine($"DLL was successfully created at: {outputDllPath}");
        }
        else
        {
            Console.WriteLine("Compilation failed!");
            foreach (Diagnostic diagnostic in result.Diagnostics)
                if (diagnostic.Severity == DiagnosticSeverity.Error)
                    Console.WriteLine(diagnostic.GetMessage());
        }
    }
}

[EgorBo]

One thing I noticed that If I spawn a new Thread with stackSize=512 on Windows then I get (int64_t)m_CacheStackBase - (int64_t)m_CacheStackLimit; = 512kb so exactly what is requested. On macOS at the same time I get 384kb here for secondary threads when I don't specify any limit

[janvorli]

@EgorBo PAL doesn't attempt to override the default stack size except for MUSL Linuxes where it is so low that it is unusable for .NET (tens of kilobytes). But if the size is specified when creating the thread, it should be honored. I am not sure if I read your comment correctly. Did you mean that when you set stack size to 512kB on macOS, you only get 384 kB?

[EgorBo]

@EgorBo PAL doesn't attempt to override the default stack size except for MUSL Linuxes where it is so low that it is unusable for .NET (tens of kilobytes). But if the size is specified when creating the thread, it should be honored. I am not sure if I read your comment correctly. Did you mean that when you set stack size to 512kB on macOS, you only get 384 kB?

I haven't tested explicit stack size on macOS (can try once I boot to it) but the default one seems to be 384kb.
I only tested the explicit on Windows and it gave me what I requested - 512kb.

I was measuring the subraction of these two on both platforms.

[janvorli]

According to Apple's doc, the default stack size for the main thread is 8MB and for the secondary threads it is 512kB.
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/CreatingThreads/CreatingThreads.html

However, the m_CacheStackLimit on macOS for secondary threads is not necessarily reliable. We get it from the pthread_get_stacksize_np and there are reports of it giving various results based on the macOS version.

[hamarb123]

According to Apple's doc, the default stack size for the main thread is 8MB and for the secondary threads it is 512kB.

Note we already don't follow the default on Windows (we went from 1MB -> 1.5MB for all threads), I see no reason to not increase it for secondary threads on macOS also since 512kB is clearly unacceptably low when we expect 128kB to be used for stack traces, and another 128kB to be available for EnsureSufficientExecutionStack.

[hamarb123]

Would a PR to change the stack size on macOS be taken? We could also make all desktop platforms have consistent stack size if that's desired too.

[janvorli]

@hamarb123 I would be happy to accept a PR that unifies default secondary stack sizes on 64 bit platforms. Even for Windows where by an accidental change the default stack size was changed from 4MB to 1.5MB about 8 years ago (see #96347).
@jkotas I wonder if we should move all the 64 bit targets to use 4MB default stack size or be more conservative. It seems that the 1.5MB that we were using before was sufficient in most of the cases (based on the low number of reports of stack overflow issues), so maybe jumping to 4MB might be unnecessarily large step.

[jaredpar]

Lowering the stack limit on Windows would almost certainly lead to customer dissatisfaction. The C# compiler is naturally recursive and it's limits in certain areas are tied to the default thread stack size. There are a non-trivial number of customers out there who run right on the edge of what we can support today (and we have extensive tests to make sure we don't regress this experience because of previous incidents). Any reduction in stack size would end up breaking our ability to build those customers code bases.

[janvorli]

@jaredpar I was definitely not suggesting to lower the current limit, we now have it set to 1.5MB, my point was whether we should move it to 4MB or just say 2MB.

[jaredpar]

@janvorli

Gotcha

I wonder if we should move all the 64 bit targets to use 4MB default stack size or be more conservative

I was reading the second half of that and interpreting "be more conservative" as potentially considering a lower stack size. Definitely happy with any change that makes Mac bigger.

[jkotas]

@jkotas I wonder if we should move all the 64 bit targets to use 4MB default stack size or be more conservative.

If macOS is the only one giving us trouble, I would fix macOS to match Windows and leave it at that.

The max stack size is a perf trade-off. The larger max stack size, the more (unused) memory is potentially committed to stacks. Growing max stack size is potentially replacing stack overflow with out of memory.

Some algorithms (like our own async - look for TryEnsureSufficientExecutionStack) try to use the stack as much as possible and gracefully cut over when the current thread stack runs out. These algorithms will make sure that the whole available stack is committed.

[janvorli]

Good point about the async stuff committing the stack space.

[hamarb123]

@jkotas I wonder if we should move all the 64 bit targets to use 4MB default stack size or be more conservative.

If macOS is the only one giving us trouble, I would fix macOS to match Windows and leave it at that.

The max stack size is a perf trade-off. The larger max stack size, the more (unused) memory is potentially committed to stacks. Growing max stack size is potentially replacing stack overflow with out of memory.

Some algorithms (like our own async - look for TryEnsureSufficientExecutionStack) try to use the stack as much as possible and gracefully cut over when the current thread stack runs out. These algorithms will make sure that the whole available stack is committed.

Do we have these sorts of problems on Linux today, which uses 8MB for every thread? It's also worth mentioning, that we are pushing people to use the stack more and more every .NET version (which I think is a good thing), e.g., with stackalloc first, recommendations to use structs instead of classes, inline arrays & collection expressions, upcoming params ReadOnlySpan<...> - all these features are slowly consuming more and more stack space for the same code.

I propose that we change Windows back to 4MB (which it used to be), or we could do 2MB for secondary threads if we think 4 is too high, and change macOS to match it:

OS	Main thread	Secondary threads
Windows	1.5MB -> 2 or 4MB	1.5MB -> 2 or 4MB
Linux	8MB	8MB
macOS	8MB	512kB -> 2 or 4MB

[jaredpar]

Some algorithms (like our own async - look for TryEnsureSufficientExecutionStack) try to use the stack as much as possible and gracefully cut over when the current thread stack runs out. These algorithms will make sure that the whole available stack is committed.

Do we have these sorts of problems on Linux today, which uses 8MB for every thread

FWIW The C# compiler uses TryEnsureSufficientExecutionStack pretty liberally across all threads. It hasn't caused any issues that I'm aware of. At the same time the compiler is a known memory hog.