x/tools/gopls: initial support for pull-based diagnostics available in LSP 3.17 #53275
Assignees
Labels
FeatureRequest
Issues asking for a new feature that does not need a proposal.
gopls/analysis
Issues related to running analysis in gopls
gopls
Issues related to the Go language server, gopls.
Tools
This label describes issues relating to any tools in the x/tools repository.
Milestone
Comments
hyangah
added
FeatureRequest
gopherbot
added
Tools
|
Change https://go.dev/cl/611843 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Sep 25, 2024
One of the unsolved problems of the gopls scalability redesign described at https://go.dev/blog/gopls-scalability is that concurrent type checking operations may perform redundant work. While we avoided redundancy within the context of *one* operation, by encapsulating the type checking batch, it is possible that there will be logically distinct operations occurring concurrently, such as computing diagnostics and autocompletion. These operations could not share type information, as they were operating within distinct type checking "realms" (recall that go/types relies on the canonicalization of imports). This CL addresses that problem by refactoring the type checking batch such that it can join two distinct operations into a shared state. The typeCheckBatch becomes a queryable entity, coordinating work across ongoing package graph traversals. This required surprisingly little change to the typeCheckBatch itself, which already abstracted the notion of a package traversal. Rather, the key missing component was a future cache implementation that was (1) retryable and (2) transient, in the sense that computed values were discarded after use. The bulk of this change is the implementation and testing of such a cache. Some elements of the refactoring remain awkward: - packageHandles are collected and merged into a shared map, because they must still be computed in large batches for efficiency. - the shared importGraph remains a clumsy optimization, requiring subtle logic to pre-build a shared import graph. My intuition is that both of these problems have an elegant solution, but this work is left for a subsequent CL. In addition to reducing CPU across the board, due to less redundancy in the existing diagnostic pass, this CL will facilitate the following additions to gopls: - Pull-based diagnostics (golang/go#53275): now that diagnostic operations are naturally joined, there is less need for gopls to manage the scheduling of the diagnostic pass. - Improvements to loading behavior (golang/go#68002): being able to handle requests asynchronously allows completion requests to potentially be handled using stale metadata. Updates golang/go#53275 Updates golang/go#68002 Change-Id: Ib228cdcce2c4b6f616d6ba5b0abeb40e87f449be Reviewed-on: https://go-review.googlesource.com/c/tools/+/611843 Reviewed-by: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
|
Change https://go.dev/cl/616835 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 7, 2024
Implement the scaffolding for pull diagnostics. For now, these are only
supported for Go files, only return parse/type errors for the narrowest
package in the default view, do not report related diagnostics, and do
not run analysis. All of these limitations can be fixed, but this
implementation should be sufficient for some end-to-end testing.
Since the implementation is incomplete, guard the server capability
behind a new internal "pullDiagnostics" setting.
Wire in pull diagnostics to the marker tests: if the server supports it
("pullDiagnostics": true), use pull diagnostics rather than awaiting to
collect the marker test diagnostics.
For golang/go#53275
Change-Id: If6d1c0838d69e43f187863adeca6a3bd5d9bb45d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/616835
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
|
Change https://go.dev/cl/618995 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 9, 2024
Add a benchmark that exercises a currently slow operation using pull diagnostics: requesting diagnostics for a large number of package files. For golang/go#53275 Change-Id: I233cfaeb4242ab44a7b46c870a195fdf7793c05c Reviewed-on: https://go-review.googlesource.com/c/tools/+/618995 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
|
Change https://go.dev/cl/619095 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 9, 2024
In CL 613715, we made import lookup lazy to mitigate the quadratic
construction of the import map during type checking. This CL makes the
equivalent change for the analysis driver.
Since analysis does not have fine-grained invalidation, it is even more
susceptible to the performance cost of this quadratic operation:
DiagnosePackageFiles-64
│ before.txt │ after.txt │
│ sec/op │ sec/op vs base │
1691.6m ± ∞ ¹ 693.6m ± ∞ ¹ -59.00% (p=0.008 n=5)
│ before.txt │ after.txt │
│ cpu_seconds/op │ cpu_seconds/op vs base │
6.480 ± ∞ ¹ 3.260 ± ∞ ¹ -49.69% (p=0.008 n=5)
For golang/go#53275
Change-Id: I8690bc85848fe1e36391d4622aa2e3d3f9878f57
Reviewed-on: https://go-review.googlesource.com/c/tools/+/619095
Auto-Submit: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
|
Change https://go.dev/cl/619516 mentions this issue: |
|
Change https://go.dev/cl/622036 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 23, 2024
Make pull diagnostics async, to optimize handling of many simultaneous requests for the diagnostics of open files. Accordingly, update the pull diagnostic benchmark to request diagnostics concurrently. Naively, these concurrent diagnostics would cause gopls to incur major performance penalty due to duplicate analyses -- as much as 5x total CPU in the pull diagnostic benchmark. To mitigate this cost, join ongoing analysis operations using a shared global transient futureCache. This reduces the benchmark back down close to its previous total CPU, with reduced latency. Eyeballing the net delta of this CL in the benchmark, it looks like +20% total CPU, -30% latency. As a nice side effect, this more than eliminates the need to pre-seed the file cache in the gopls marker tests. The shared futures provide more consolidation of work than the pre-seeding, because of variance in the cache keys of standard library packages, due to different gopls options. For golang/go#53275 Change-Id: Ie92bab4c140e3f86852531be8204b6574b254d8e Reviewed-on: https://go-review.googlesource.com/c/tools/+/622036 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
|
Change https://go.dev/cl/622037 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 24, 2024
analysisNode.cacheKey shows up as very hot in benchmarks, yet analysis should observe the same invalidation heuristics as other package data. Memoizing the cache keys in the snapshot reduced total CPU of BenchmarkDiagnosePackageFiles by 10-15%. For golang/go#53275 Change-Id: I0f60c3e78c77ac6e58b14308bb0331022babae69 Reviewed-on: https://go-review.googlesource.com/c/tools/+/622037 Auto-Submit: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
|
Change https://go.dev/cl/622038 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 28, 2024
For the purposes of go/analysis, gopls could not skip syntactic object resolution, as it is part of the go/analysis contract. This prevents analysis from reusing existing type-checked packages, leading to increased CPU and memory during diagnostics. Fix this by making object resolution lazy, and ensuring that all analysed files are resolved prior to analysis. This could introduce a race if gopls were to read the fields set by object resolution, for example if it was printing the tree using ast.Fprint, so we include a test that these fields are only accessed from packages or declarations that are verified to be safe. Since the resolver is not separate from the parser, we fork the code and use go generate to keep it in sync. For golang/go#53275 Change-Id: I24ce94b5d8532c5e679789d2ec1f75376e9e9208 Reviewed-on: https://go-review.googlesource.com/c/tools/+/619516 Reviewed-by: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 28, 2024
Consolidate the logic of type checking and analysis, for the following benefits: - Simplify by eliminating redundant logic to type check and import packages in the analysis driver. - Reduce work by reusing type checked packages in analysis. By the time we run analysis on open packages, we likely have type checked the packages already, so the work to type check inside the analysis driver is very much redundant. - Reduce work by reusing the package key from packageHandle (which we have gone to great pains to optimize). - Reduce work (and file cache space) by avoiding the need to store export data alongside analysis facts. - Leverage the precision of our reachability analysis by using a bloom filter of reachable packages to better filter facts. - This indirectly fixes golang/go#64227 and golang/go#64236, since the crashing logic is deleted. For golang/go#53275 Fixes golang/go#64227 Fixes golang/go#64236 Change-Id: I431b8da35b2dce7c63f56ec1a3727e0747b79740 Reviewed-on: https://go-review.googlesource.com/c/tools/+/622038 Reviewed-by: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
findleyr
changed the title
|
For now, I'm going to restrict this to "initial support", because while there is more to do, we have enough to get started integrating with internal systems that need pull diagnostic support. I'll open up a follow-up issue for more TODO items. Therefore, this is done. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
LSP 3.17 adds support for Pull Diagnostics
The spec includes Document Diagnostics, Workspace Diagnostics.
I think this allows us to address some long-standing issues like
(some users want to see diagnostics only when they save a file, not as they type
Partly
The text was updated successfully, but these errors were encountered: