Handle exceptions from parallel ReadyToRun compilation - #132282
Conversation
The basic bug here is that ComputeMarkedNodes may throw and, if it does, the worker threads are not correctly stopped and cleaned up. This change refactors the worklist logic into its own data structure and provides a single Dispose call for cleaning up. The bug fix is very simple, but the existing code was complicated enough that it was difficult to immediately see it was correct.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f880f9c-8295-4ade-b4f1-fc9031473fdc
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| List<EcmaModule> rootingModules = new List<EcmaModule>(); | ||
| HashSet<EcmaModule> crossModuleInlineableCode = new HashSet<EcmaModule>(); | ||
| bool determinismCheckFailed; | ||
| using var compilation = (ReadyToRunCodegenCompilation)BuildCompilation( |
There was a problem hiding this comment.
GH diff is kind of failing here -- VS Code is better. It shows that I just extracted the real work into the new BuildCompilation method so that I could using using var compilation instead of manual Dispose and try-finally
| @@ -402,9 +401,8 @@ public override void Compile(string outputFile) | |||
| { | |||
| _dependencyGraph.ComputeMarkedNodes(); | |||
There was a problem hiding this comment.
This was the actual source of an exception. It should now be handled by the Dispose on this class, which is driven by the using in Program.cs.
There was a problem hiding this comment.
Pull request overview
This PR refactors parallel ReadyToRun compilation to centralize work distribution/worker lifetime management in a new CompilationWorklist<TWorkerState>, improving exception handling and cleanup behavior, and adjusts crossgen2 compilation ownership to ensure deterministic disposal.
Changes:
- Introduces
CompilationWorklist<TWorkerState>to coordinate parallel work and propagate the first unexpected worker exception to the coordinating thread. - Refactors
ReadyToRunCodegenCompilationto use per-worker state and dispose worker/JIT state earlier to reduce peak memory during object emission. - Refactors crossgen2 to build compilations via a helper and use scoped (
using var) disposal for safer cleanup.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj | Adds the new CompilationWorklist.cs to the build. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs | Replaces bespoke thread/semaphore orchestration with CompilationWorklist, adds per-worker state tracking, and disposes worklist before emission. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/CompilationWorklist.cs | New helper to manage parallel work distribution, worker lifetime, and exception capture/rethrow. |
| src/coreclr/tools/aot/crossgen2/Program.cs | Refactors compilation creation into BuildCompilation and uses scoped disposal to simplify ownership and cleanup. |
Review details
Suppressed comments (1)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/CompilationWorklist.cs:60
- Run(...) should validate that the provided items list is non-null before storing it and using items.Count in TryTake; otherwise a null argument would crash later with a NullReferenceException.
public void Run(IReadOnlyList<DependencyNodeCore<NodeFactory>> items)
{
EnsureWorkers();
Debug.Assert(_items is null);
_items = items;
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| DependencyNodeCore<NodeFactory> item, | ||
| ref TWorkerState workerState); | ||
|
|
||
| internal sealed class CompilationWorklist<TWorkerState> : IDisposable |
There was a problem hiding this comment.
NativeAOT has equivalent logic implemented using Parallel.ForEach:
runtime/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs
Lines 166 to 169 in 2580b59
Can we use that instead of implementing our own custom MT scheduler?
In any case, both R2R and NAOT should be on the same plan if we are refactoring this.
There was a problem hiding this comment.
Makes sense to me. I was starting with refactoring, but keeping the existing scheduler, but I can go further and try to get us down to just Parallel.ForEach.
Summary
CompilationWorklistTesting
ILCompiler.ReadyToRun.Tests: 57 passed, 2 skipped, 0 failedNote
This pull request description was generated with GitHub Copilot.