Repository navigation
perf(diagnostics): share analyzer results across document requests - #404
razzmatazz merged 2 commits into
Conversation
|
|
||
| type private SolutionAnalysisCache = ConcurrentDictionary<ProjectId, Lazy<Task<ImmutableArray<Diagnostic>>>> | ||
|
|
||
| let private solutionAnalysisCaches = |
There was a problem hiding this comment.
not a fan of this global cache. can we add this to wf structure? unsure how state transfers/caching/locking would work though
There was a problem hiding this comment.
not a fan of this global cache. can we add this to wf structure? unsure how state transfers/caching/locking would work though
What exactly do you mean by "add this to wf"? Do you mean:
LspWorkspaceFolderholds the cache, which is then plugged into the WorkspaceFolder life-cycle by being created at solution load, and dropped in teardown? This would be a big improvement, I agree.- The cache itself is objectionable and we should just thread the record data through
LspWorkspaceFolderUpdateFn? This would gut the PR, because it'll no longer cache the in-flightTaskproperly. The UpdateFns apply only after the handlers are evaluated, so the burst of diagnostics will always just see an empty set of diagnostics and re-request them all again.
There was a problem hiding this comment.
Went ahead with option 1 there since Option 2 should just close the PR/Issue as rejected
There was a problem hiding this comment.
I just have one question, re concurrency below
| cache.GetOrStartAnalysis( | ||
| project.Solution, | ||
| project.Id, | ||
| fun () -> | ||
| // Cancellation applies to each waiter, not to the shared analysis. | ||
| let cwa = compilation.WithAnalyzers(analyzers, project.AnalyzerOptions) | ||
| cwa.GetAllDiagnosticsAsync(CancellationToken.None) | ||
| ) |
There was a problem hiding this comment.
Q: how does this work in face of concurrent requests for the same document/project? i.e. would there still be two concurrent diagnostics recomputes–and a semaphore is needed to avoid that? or does that not happen?
There was a problem hiding this comment.
They can independently construct the same Lazy<> wrapped Task and add it to the ConcurrentDictionary, but they don't compute the task multiple times. No semaphore needed because they'll all converge on the same thread-safe .Value call. This is covered by the testProjectAnalyzerAnalysisIsSharedAcrossDocumentsAndRequestCancellation test.
|
Just getting back to this PR–is there a particular observable-by-human performance or correctness issue that this PR addresses? The reason I am somewhat reluctant to merge it is that this adds quite a bit of complexity for unclear (to me) wins. |
Yes, the benchmark in the PR is a real benchmark that shows a ~0.5-2s before change wait time for 12 documents going down to ~50 ms on my machine when waiting for diagnostics with analysers enabled. I've been using this build for a while now and it's been a significant improvement when using it in Neovim. The actual production code is +43/-18, most of the PR is tests/benchmarks. |
Move the shared analyzer cache from a module-level global into LspWorkspaceFolder so it is reachable from ServerState and released by workspaceFolderTeardown rather than when the GC clears the weak keys. Locking, invalidation, and retention are unchanged: the ConditionalWeakTable moves intact into AnalyzerDiagnosticsCache, still keyed by immutable Solution snapshot.
ed8c529 to
f6de893
Compare
| cache.GetOrStartAnalysis( | ||
| project.Solution, | ||
| project.Id, | ||
| fun () -> | ||
| // Cancellation applies to each waiter, not to the shared analysis. | ||
| let cwa = compilation.WithAnalyzers(analyzers, project.AnalyzerOptions) | ||
| cwa.GetAllDiagnosticsAsync(CancellationToken.None) | ||
| ) |
Fixes #403.
Diagnosis
Document diagnostics currently create a new
CompilationWithAnalyzersand runGetAllDiagnosticsAsyncfor every requested document. The result covers the whole project, but each request keeps only diagnostics for its own syntax tree. Pulling several documents from one unchanged solution snapshot therefore repeats the same project-wide analyzer work.Change
Cache one lazy analyzer task per project and immutable Roslyn
Solutionsnapshot. Document and compilation diagnostics share that result. A new solution snapshot naturally gets a new cache, and the weak solution key allows old snapshots to be collected.Request cancellation only cancels that request's wait. It does not cancel analyzer work already shared with another document request.
The production change is 43 added lines and 18 removed lines in
Roslyn/Analyzers.fs.User impact
Diagnostic payloads and update behavior do not change. The first analyzer request for a solution snapshot still performs the full analysis. Later document requests for the same snapshot reuse it. Editing any document creates a new snapshot, so the next request analyzes the updated project before reuse begins again.
A real-LSP four-file evaluation covered same-file edits that clear and add IDE diagnostics, saves, unrelated document pulls, cross-file edits that add and clear
CS0103, workspace pulls, and push diagnostics. Normalized diagnostic payloads matched the baseline in all three fresh sessions.Benchmark
tests/benchmarks/AnalyzerDiagnostics.fsxstarts the real LSP, enables analyzers, warms one document, then pulls diagnostics for 12 other documents in the same project snapshot. It prints the actual request times and diagnostic counts.Upstream main (
e2efc47), representative run:This branch (
c2002e0), representative run:Across three fresh server processes, the median wall time fell from 572 ms to 27 ms. Median server CPU time fell from 2,150 ms to 30 ms. Every sample returned the same 72 diagnostics.
Verification
The analyzer cache regression test also covers concurrent document requests, request cancellation isolation, completed-result reuse, and invalidation after a document edit.