Repository navigation
Improve background progress reporting - #1545
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation propagation, configuration handling, duplicate-operation tracking, and one test assertion are currently incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces shared LSP progress reporting for typechecking, analyzers, project loading, and dependent-file checks.
Changes:
- Adds shared, cancellable batch progress infrastructure.
- Integrates reporters into server workflows.
- Adds concurrency helpers and reporter tests.
File summaries
| File | Description |
|---|---|
FSharpLspClient.fs |
Implements shared progress reporting. |
FSharpLspClient.fsi |
Exposes the reporter API. |
AdaptiveServerState.fs |
Integrates shared progress and cancellation. |
Utils.fs |
Adds percentage-based parallelism helpers. |
Utils.fsi |
Declares new parallelism helpers. |
SharedTypecheckProgressReporterTests.fs |
Tests reporter behavior. |
Program.fs |
Registers the new tests. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let simpleName = IO.Path.GetFileName fileName | ||
| activeFiles <- Set.add simpleName activeFiles |
| let projectLoadingProgressReporter = | ||
| new SharedTypecheckProgressReporter("Loading Projects", fun () -> progressLookup.CreateProgressReport(lspClient)) |
| opts, | ||
| shouldCache = shouldCache | ||
| ) | ||
| |> Async.withCancellation progressCt |
| fileToken, | ||
| progressReporter.CancellationToken | ||
| ) | ||
| CancellationTokenSource.CreateLinkedTokenSource(rootToken, fileToken, progressCt) |
|
|
||
| if config.Notifications.BackgroundServiceProgress then | ||
| do! progress.Begin($"Checking for unnecessary parentheses {fileName}...", message = filePathUntag) | ||
| use! _progress = analyzerProgressReporter.Begin($"Unnecessary parens - {fileName}") |
| // Second batch — counters should have been reset | ||
| let! batchDisp2 = | ||
| reporter.BeginBatch ([| "C:/src/C.fs"; "C:/src/D.fs" |]) CancellationToken.None | ||
| |> Async.AwaitTask | ||
|
|
||
| do! settle () | ||
|
|
||
| // The Begin for second batch should show 0% (0/2) | ||
| let callsArr = calls.ToArray() | ||
|
|
||
| let hasZeroPercentage = | ||
| callsArr | ||
| |> Array.exists (fun c -> | ||
| match c with | ||
| | Begin(_, _, Some 0u) -> true | ||
| | Report(_, Some 0u) -> true | ||
| | _ -> false) |
| Expect.isTrue (ct = CancellationToken.None) "CancellationToken should be None after all files end" | ||
| } | ||
|
|
||
| testCaseAsync "Multiple sequential files reuse the same report cycle" |
| let normalizedFiles = files |> Array.map normalizePath |> Array.distinct | ||
| batchTotal <- batchTotal + normalizedFiles.Length | ||
| batchFiles <- Set.union batchFiles (normalizedFiles |> Set.ofArray) |
69e74d3 to
d7e1661
Compare
nojaf
left a comment
nojaf
left a comment
There was a problem hiding this comment.
Thanks for this! Folding the per-file "Typechecking X" / "Checking unused opens X" notifications into one progress item is a real UX improvement, and it also saves the window/workDoneProgress/create round-trip per typecheck. I'd like to get it in, but a few things need another look first.
Behaviour changes
1. Most users would no longer see typecheck and project-load progress.
On main, "Typechecking Foo.fs", "Typechecking Dependent F# files" and "Loading N Projects" are always reported. This PR gates all three behind BackgroundServiceProgress, which defaults to false (LspHelpers.fs), and Ionide doesn't expose FSharp.notifications.backgroundServiceProgress in its settings. So for nearly every VS Code user, project loading and typechecking would go silent. If that's intentional, could you call it out in the description and changelog? Personally I'd keep at least "Loading Projects" always on.
2. Dependent-file checks go from parallel75 to parallel25.
On a 4-core machine that means one file at a time, so typechecking dependents after a save gets roughly 3x slower. This feels unrelated to progress reporting. Could it move to its own PR with some numbers behind it? parallel50 is also added but unused.
3. Cancelling now affects every operation, and keeps affecting new ones for a while.
Because the report is shared, the client's cancel button now cancels every typecheck in flight, including the one for the file being edited. Before, it only cancelled that one file. The cancelled report also stays in progressReport until all scopes are done, so any typecheck that starts in that window gets an already-cancelled token from GetCancellationToken() and fails straight away. Maybe Begin could drop a report whose CTS has fired and create a fresh one?
4. The project-loading percentage doesn't move.
BeginBatchSync starts every project up front and ends them all in Dispose, so the user sees "0/N completed" with a list of .fsproj names for the whole load, followed by a jump to the end. Real progress would need per-project events from the loader (loader.Notifications). Otherwise I'd keep the old "Loading N Projects" message without a percentage.
Smaller things
- Cancellation now escapes from the dependent-file check. With the new
whenfilter, cancellation ofrootToken/progressCtescapesbypassAdaptiveAndCheckDependenciesForFile, andSaveDocumentawaits that. A quick second save or edit now ends the save handler with an exception thatmainswallowed. Worth checking that it gets logged quietly rather than as an error. - The lock is held during a client round-trip.
StartFileholds the lock whilereport.Beginwaits for the client's response toworkDoneProgress/create, so other typechecks queue behind it. It only matters when the setting is on, but it's on the hot path. GetCancellationToken()takes the lock even when reporting is disabled (the default), on every typecheck. It could returnCancellationToken.Noneearly whenisEnabled()is false.BeginBatchSyncdoesn't await its tasks. TheStartFile/EndFile/EndBatchtasks are discarded, so any exceptions (e.g.ObjectDisposedExceptionat shutdown) go unobserved. It also sends 2N$/progressmessages for N projects.
Nitpicks
- The name is too narrow.
SharedTypecheckProgressReporteris also used for analyzers and project loading.SharedProgressReporterwould be more accurate. - Labels are passed where paths are expected.
Begin("Unused opens - Foo.fs")passes a label throughPath.GetFullPath/GetFileName, which only works by accident. A separate label parameter, or a non-path key, would be cleaner. - The
Utils.fschange dropped a doc comment. Theparallel75doc comment is gone (the.fsicopy stays), and there's a stray extra blank line. - The test mock parses JSON by hand. Its
IndexOf/Substringcode could be replaced withp.Value.["kind"],p.Value.["message"]etc., sincep.Valueis already aJToken.
…hared progress reporter
…g in typechecking
Rename SharedTypecheckProgressReporter to SharedProgressReporter and rework it around scopes: - Begin(label) and BeginBatch(count) return scopes that carry the cancellation token of their report, so GetCancellationToken is gone and a disabled reporter takes no lock. - A single agent sends all progress messages to the client in order. Callers only change the state under a lock and never wait for the workDoneProgress/create round-trip. Bursts are coalesced and nothing is left unawaited. - When the user cancels the report, the next operation gets a fresh report and token. Operations that a cancelled batch starts later get the cancelled token, so the batch stops. - A batch counts its own operations instead of file paths. A batch that ends early stops counting its operations that never ran. Batch operations have no label: the typecheck they run shows the file name once it holds the typecheck lock, so queued files do not show. - Labels are shown as is instead of passing through Path.GetFullPath. The typecheck itself does not use the progress token, as on main: a cancelled check of an open file would stay in the adaptive cache until the file changes. The dependent-file batch links the batch token, so cancelling the report stops that batch. Keep the behaviour of main where the review asked for it: - Typecheck progress is always shown again. Only analyzer progress follows FSharp.notifications.backgroundServiceProgress. - Project loading shows "Loading N Projects" as before. - Dependent files are checked with parallel75 again, and parallel50, parallel25 and the lost doc comment are reverted. A cancelled dependent-file check is logged at debug level instead of failing the save handler. Analyzers log a cancel at debug level, not error. Port the reporter tests to the new API, parse the progress payload as a JToken and assert only on the calls a step adds.
7dbf534 to
d454d46
Compare
|
@TheAngryByrd I rebased this on main and pushed a commit on top (d454d46) that works through the review feedback. Here is what changed, and one open question for you. What changed
Open question: percentage vs. the LSP specThe shared report can break two rules for
Options I see:
Which would you prefer? If you had something else in mind for any of the changes above, let me know and we can adjust. |
|
Id say accept, reducing popups/notifications was the key point of this PR |
ServerProgressLookup.Cancel ran the token callbacks while holding the progress table lock, while SharedProgressReporter takes that lock through createReport while holding its own. No callback disposes a scope synchronously today, but the opposite lock order was one such callback away from a deadlock. Cancel now removes the entry under the lock and cancels after releasing it. A report whose operations started and ended between two syncs of the agent was never sent, so the agent never disposed it either. The reporter now disposes such a report when it retires it.
|
@TheAngryByrd thanks, accepting the percentage behaviour it is. A second review pass turned up a few more things. Already pushed in 9b59154:
Coming in the next push:
Things I left as they are, unless you see it differently:
Is the analyzer batch change OK with you, and does anything in the second list need changing? |
The built-in analyzers run one at a time behind analyzersLocker. Each one opened its own scope after taking the lock and closed it before releasing it, so the shared "Analyzing" report ended between two analyzers and the next one started a new report. A cancel also only stopped the analyzer that was running. The analyzers of a file now run in one batch. Each analyzer joins the batch before it waits for the lock and shows its label while it runs. A cancel of the report stops the analyzers that did not run yet.
|
Analyzer batch looks good
So the notification is stuck? Sounds like a bug
Depends on how much infrastructure and value we get from the tests |
|
@TheAngryByrd thanks.
Not stuck: the report still ends when the last operation ends, and the message keeps listing what runs. Only the percentage was stale. Example: on save the built-in analyzer batch finishes while the external analyzers still run, so the client showed "External analyzers - Foo.fs" at 100% until they finished. LSP has no way to remove a percentage from a report, only to send a new value, so I agree it's a bug. Fix (in the next push): when the last batch ends, the operations that still run join the count, and operations that start later count too. That example now shows "External analyzers - Foo.fs (4/5 completed)" at 80% and ends when they finish. It stays one report. The cost is a drop at the moment the batch ends (100% to 80% here), the same kind of drop as when a second batch joins. Two reporter tests cover it.
I have to correct what I wrote before: the LSP test harness already records every notification and request, What I think is worth it:
What I would skip: a test that Cancel stops the dependent batch. The cancel has to land while the batch runs, so it needs a project large enough to be slow, and it would be timing-dependent. Shall I add the first two? |
A progress report cannot drop its percentage. When the last batch of a shared report ended while labelled operations still ran (an open file check, the external analyzers), the client kept the last percentage, usually 100%, until the report ended. The operations that still run now join the count when the last batch ends, and operations that start later count too. The percentage stays below 100% until the report ends. Counters no longer reset between batches of one report, so a new batch adds to the count. Add the changelog entry for 0.86.1, so merging the PR releases it.
Summary
FSharp.notifications.backgroundServiceProgressfor all shared progress reportersTesting
dotnet fantomas --check build.fsx srcdotnet build test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj -c Release -f net10.0 -p:ImportDirectoryPackagesProps=false --nologodotnet run -c Release -f net10.0 --no-build --project test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj -- --filter-test-list "SharedTypecheckProgressReporter"— 17 passedThe full solution build reached the changed projects, but nested TestExplorer sample builds imported a parent
Directory.Packages.propsand failed withNU1008. The focused LSP build completed with zero warnings and errors.