Repository navigation
fix(compiler): bound compile CPU on a dedicated-thread IIoPool lane #5375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ internal sealed class MeshNodeLanguageService : IMeshLanguageService | |
| // inline, inside this leaf's Compile-pool gate, with no cancellation participation at all, | ||
| // so a silo/mesh teardown racing the first caller could not reclaim that gate permit within | ||
| // the drain budget. Every caller now races its OWN `ct` against this shared Task via | ||
| // Task.WaitAsync (see GetScriptCompletionsAsync/GetScriptDiagnosticsAsync) — the FIRST | ||
| // Task.WaitAsync (see GetScriptCompletionsAsync/ScriptCompilationAsync) — the FIRST | ||
| // caller's own token never cancels the build (it is shared by every request against this | ||
| // mesh instance), it only governs how long THAT caller waits. | ||
| private readonly Lazy<Task<ScriptWorkspace>> _scriptWorkspace; | ||
|
|
@@ -86,6 +86,11 @@ internal sealed class MeshNodeLanguageService : IMeshLanguageService | |
| // with ConfigureAwait(false) behind a gate. Falls back to the unbounded pool | ||
| // when no registry is wired (e.g. tests constructing the service outside DI). | ||
| private readonly IIoPool _ioPool; | ||
| private readonly IIoPool _cpuLane; | ||
|
|
||
| /// <summary>Test seam (InternalsVisibleTo): runs on the thread that binds a diagnostics compilation. | ||
| /// Null in production.</summary> | ||
| internal Action? OnDiagnoseStarting { get; set; } | ||
|
|
||
| public MeshNodeLanguageService( | ||
| MeshNodeCompilationService compilationService, | ||
|
|
@@ -104,6 +109,11 @@ public MeshNodeLanguageService( | |
| memoryStore = new CompletionMemoryStore(hub, logger); | ||
| _ioPool = hub.ServiceProvider.GetService<IoPoolRegistry>()?.Get(IoPoolNames.Compile) | ||
| ?? IoPool.Unbounded; | ||
| // The CPU lane: every GetDiagnostics (Roslyn's bind — the expensive part of a squiggle | ||
| // request) runs on a bounded set of DEDICATED threads, never a ThreadPool worker | ||
| // (Doc/Architecture/CompileOffTheThreadPool). The async workspace legs stay on _ioPool. | ||
| _cpuLane = hub.ServiceProvider.GetService<IoPoolRegistry>()?.Get(IoPoolNames.CompileCpu) | ||
| ?? IoPool.Unbounded; | ||
|
|
||
| // 🚨 The two helpers own subscriptions that outlive the request that started them — the | ||
| // usage index's mesh-wide corpus scan (its own 30 s Timeout, a 1 s Throttle) and the memory | ||
|
|
@@ -140,7 +150,8 @@ public IObservable<NodeDiagnosticsOutcome> GetDiagnostics(string nodeTypePath) | |
| // no diagnostics, which is outside #1592's failure (the mandated sweep | ||
| // enumerates nodeType:NodeType) but is not what this branch means. | ||
| ? Observable.Return(NodeDiagnosticsOutcome.NotCompilable) | ||
| : _ioPool.Run(ct => GetDiagnosticsAsync(cached, ct)) | ||
| : _ioPool.Run(ct => WorkspaceCompilationAsync(cached, ct)) | ||
| .SelectMany(compilation => OnCpuLane(compilation, DiagnoseWorkspace)) | ||
| .Select(NodeDiagnosticsOutcome.Compiled)), | ||
|
|
||
| // Genuinely not there, or its delete is in flight — either way nothing was checked. | ||
|
|
@@ -280,13 +291,15 @@ private IObservable<NodeDiagnosticsOutcome> SpeculativeFor( | |
| // this says "nothing was compiled" rather than inventing a clean bill. | ||
| ? Observable.Return(NodeDiagnosticsOutcome.NotCompilable) | ||
| : _ioPool.Run(ct => | ||
| speculativeCompilation.GetDiagnosticsAsync(inputs, sourcePath, proposedCode, ct)) | ||
| speculativeCompilation.CreateCompilationAsync(inputs, sourcePath, proposedCode, ct)) | ||
| .SelectMany(compilation => OnCpuLane(compilation, SpeculativeCompilation.Diagnose)) | ||
| .Select(NodeDiagnosticsOutcome.Compiled)), | ||
| // A non-NodeType owner that EXISTS → a standalone script Code node: diagnose in | ||
| // the script environment so lesson cells get live squiggles too (this used to | ||
| // fall through to a compilation that parses a cell as REGULAR C#, reporting the | ||
| // spurious "top-level statements must be in an executable" on every cell). | ||
| _ => _ioPool.Run(ct => GetScriptDiagnosticsAsync(proposedCode, ct)) | ||
| _ => _ioPool.Run(ct => ScriptCompilationAsync(proposedCode, ct)) | ||
| .SelectMany(compilation => OnCpuLane(compilation, DiagnoseScript)) | ||
| .Select(NodeDiagnosticsOutcome.Compiled), | ||
| }; | ||
|
|
||
|
|
@@ -536,14 +549,32 @@ private static bool VersionsEqual( | |
| return true; | ||
| } | ||
|
|
||
| private static async Task<IReadOnlyList<DiagnosticInfo>> GetDiagnosticsAsync( | ||
| CachedWorkspace cached, CancellationToken ct) | ||
| /// <summary> | ||
| /// Hands <paramref name="compilation"/> to the CPU lane for its bind. A null compilation (no | ||
| /// project / document) answers empty without taking a lane slot. | ||
| /// </summary> | ||
| private IObservable<IReadOnlyList<DiagnosticInfo>> OnCpuLane<TCompilation>( | ||
| TCompilation? compilation, | ||
| Func<TCompilation, CancellationToken, IReadOnlyList<DiagnosticInfo>> diagnose) | ||
| where TCompilation : class | ||
| => compilation is null | ||
| ? Observable.Return<IReadOnlyList<DiagnosticInfo>>(Array.Empty<DiagnosticInfo>()) | ||
| : _cpuLane.InvokeBlocking(ct => | ||
| { | ||
| OnDiagnoseStarting?.Invoke(); | ||
| return diagnose(compilation, ct); | ||
| }); | ||
|
|
||
| /// <summary>The IO half of a node workspace's diagnostics: the project's compilation (not yet bound).</summary> | ||
| private static async Task<Compilation?> WorkspaceCompilationAsync(CachedWorkspace cached, CancellationToken ct) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No change. |
||
| { | ||
| var project = cached.Workspace.CurrentSolution.GetProject(cached.ProjectId); | ||
| if (project is null) return Array.Empty<DiagnosticInfo>(); | ||
| return project is null ? null : await project.GetCompilationAsync(ct).ConfigureAwait(false); | ||
| } | ||
|
|
||
| var compilation = await project.GetCompilationAsync(ct).ConfigureAwait(false); | ||
| if (compilation is null) return Array.Empty<DiagnosticInfo>(); | ||
| /// <summary>The CPU half: bind and report — a CPU-lane leaf (no IO, no pool call).</summary> | ||
| private static IReadOnlyList<DiagnosticInfo> DiagnoseWorkspace(Compilation compilation, CancellationToken ct) | ||
| { | ||
| var diags = compilation.GetDiagnostics(ct); | ||
| if (diags.IsDefaultOrEmpty) return Array.Empty<DiagnosticInfo>(); | ||
|
|
||
|
|
@@ -645,19 +676,20 @@ private async Task<IReadOnlyList<CompletionEntry>> GetScriptCompletionsAsync( | |
| /// Diagnostics for a standalone SCRIPT Code node — the script-environment sibling of | ||
| /// <see cref="SpeculativeCompilation"/>, so lesson cells get live squiggles. | ||
| /// </summary> | ||
| private async Task<IReadOnlyList<DiagnosticInfo>> GetScriptDiagnosticsAsync( | ||
| string proposedCode, CancellationToken ct) | ||
| private async Task<Compilation?> ScriptCompilationAsync(string proposedCode, CancellationToken ct) | ||
| { | ||
| var script = await _scriptWorkspace.Value.WaitAsync(ct).ConfigureAwait(false); | ||
| var document = script.Workspace.CurrentSolution | ||
| .WithDocumentText(script.DocumentId, SourceText.From(StripKernelDirectives(proposedCode))) | ||
| .GetDocument(script.DocumentId); | ||
| var compilation = document is null | ||
| return document is null | ||
| ? null | ||
| : await document.Project.GetCompilationAsync(ct).ConfigureAwait(false); | ||
| if (compilation is null) return Array.Empty<DiagnosticInfo>(); | ||
|
|
||
| } | ||
|
|
||
| /// <summary>The CPU half of a script's diagnostics — a CPU-lane leaf.</summary> | ||
| private static IReadOnlyList<DiagnosticInfo> DiagnoseScript(Compilation compilation, CancellationToken ct) | ||
| { | ||
| var diags = compilation.GetDiagnostics(ct); | ||
| if (diags.IsDefaultOrEmpty) return Array.Empty<DiagnosticInfo>(); | ||
| var result = new List<DiagnosticInfo>(); | ||
|
|
@@ -968,7 +1000,10 @@ private static async Task<ScriptWorkspace> BuildScriptWorkspaceAsync(IServicePro | |
| language: LanguageNames.CSharp, | ||
| compilationOptions: new CSharpCompilationOptions( | ||
| OutputKind.DynamicallyLinkedLibrary, | ||
| metadataReferenceResolver: Kernel.Hub.MeshScriptEnvironment.MetadataResolver) | ||
| metadataReferenceResolver: Kernel.Hub.MeshScriptEnvironment.MetadataResolver, | ||
| // Roslyn's default fans every bind out onto the ThreadPool — see | ||
| // EmitPipeline.CreateRunCompilationOptions / CompileOffTheThreadPool. | ||
| concurrentBuild: false) | ||
| .WithUsings(Kernel.Hub.MeshScriptEnvironment.Imports), | ||
| parseOptions: new CSharpParseOptions(kind: SourceCodeKind.Script), | ||
| metadataReferences: references, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,19 @@ public async Task<IReadOnlyList<DiagnosticInfo>> GetDiagnosticsAsync( | |
| string sourcePath, | ||
| string proposedCode, | ||
| CancellationToken ct) | ||
| => Diagnose(await CreateCompilationAsync(inputs, sourcePath, proposedCode, ct).ConfigureAwait(false), ct); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The language service now does what you suggest: |
||
|
|
||
| /// <summary> | ||
| /// The IO half of <see cref="GetDiagnosticsAsync"/>: resolves any proposed NuGet references and | ||
| /// builds the speculative compilation (parse only — nothing is bound yet). Split from | ||
| /// <see cref="Diagnose"/> so a caller can run the CPU-bound half on the dedicated CPU lane | ||
| /// (<c>IoPoolNames.CompileCpu</c>, <c>Doc/Architecture/CompileOffTheThreadPool</c>). | ||
| /// </summary> | ||
| public async Task<CSharpCompilation> CreateCompilationAsync( | ||
| CompilationInputs inputs, | ||
| string sourcePath, | ||
| string proposedCode, | ||
| CancellationToken ct) | ||
| { | ||
| // Strip #r from the proposed source so Roslyn doesn't reject it with CS7011. | ||
| // Resolve any new package refs and append them to the cached reference set — | ||
|
|
@@ -106,12 +119,20 @@ public async Task<IReadOnlyList<DiagnosticInfo>> GetDiagnosticsAsync( | |
| SourceText.From(code), inputs.ParseOptions, path: path)); | ||
| } | ||
|
|
||
| var compilation = CSharpCompilation.Create( | ||
| return CSharpCompilation.Create( | ||
| inputs.AssemblyName, | ||
| syntaxTrees: trees, | ||
| references: effectiveReferences, | ||
| options: inputs.CompilationOptions); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The CPU-bound half of <see cref="GetDiagnosticsAsync"/>: binds <paramref name="compilation"/> | ||
| /// and reports its user-actionable diagnostics (framework-generated trees filtered out). | ||
| /// Synchronous and pure — no IO, no pool call — so it is a valid CPU-lane leaf. | ||
| /// </summary> | ||
| public static IReadOnlyList<DiagnosticInfo> Diagnose(CSharpCompilation compilation, CancellationToken ct) | ||
| { | ||
| var diags = compilation.GetDiagnostics(ct); | ||
| if (diags.IsDefaultOrEmpty) return Array.Empty<DiagnosticInfo>(); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
awaitis not new.CompileAsyncCorewas already anasync Taskcompile leaf (NuGet restore, debug source write). #5327 awaitedCompileThread.Runat this exact spot, and this PR swaps that for the lane.OnThreadPool(Func<Task<T>>)(Task.Run), never on a hub or grain thread.OnThreadPoolre-establishesImpersonateAsSysteminside the async lambda for its whole span, andEmitCompileddoes no mesh reads.ObservableAwait.Await(ct)(RunContinuationsAsynchronously), notToTask/await observable.Turning the whole compile leaf reactive means replacing the
CompileAsync/_inflightCompilesTaskshape. That refactor is unrelated to bounding CPU, so I've kept it out of this PR. No change here.