x/tools/gopls: cold goimports cache causes slow code actions on first save #44863
Comments
gopherbot
added
Tools
|
Another potential solution to the underlying problem could be to change the timing of our imports cache warm-up. Currently it's kicked off at first usage (unimported completion, code action, etc.). Maybe we could/should just warm up the cache immediately following initialization. |
|
After some discussion, we've agreed to try warming up the imports cache after the first |
|
Thanks very much for raising this, @findleyr
This would help compared to the situation today, but would still represent unnecessary processing of the module cache each time a new view is established (and then a period refresh for each view). If however, per your suggestion, we shared the goimports directory cache across views and kicked off a scan of that on initialisation (in the case (I can imagine that when |
Per my understanding, we're talking about ~seconds per session (please correct me if I'm missing something). Assuming that most editing of Go files takes more than a few seconds, warming the cache on the first didOpen should almost entirely mitigate the user facing aspects of this, while avoiding unnecessary work when simply opening an LSP session that might not be used. Changing the goimports caching model is a risky change and has non-trivial maintenance burden (for example, if/when we add better support for build tags). Let's start with pre-warming the cache and then re-evaluate. While I agree that it would be principled to share the cache, we have other principled improvements that are much more user-facing (for example preserving stale package metadata, working on the parser, or improving support for single-file editing). |
findleyr
changed the title
|
Change https://go.dev/cl/559496 mentions this issue: |
Now that views are immutable, we can simplify the invalidation of imports state. The imports.ProcessEnv can also be immutable, and we need only invalidate module state (i.e. dependencies) if a go.mod file changes. While at it, add documentation and perform some superficial cleanup in the internal/imports package. This is a first step toward larger state optimizations needed for the issues below. TODOs are added for remaining work. For golang/go#44863 For golang/go#59216 Change-Id: I23c1ea96f241334efdbfb4c09f6265637b38f497 Reviewed-on: https://go-review.googlesource.com/c/tools/+/559496 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
|
Change https://go.dev/cl/559636 mentions this issue: |
|
Change https://go.dev/cl/561436 mentions this issue: |
In CL 508506, we missed a performance regression because there were no benchmarks for goimports' initial scan: the only scan benchmark related to re-scanning, which doesn't recursively descend into all module cache directories. Add a benchmark for the initial scan. For golang/go#44863 Change-Id: I94ec8b2b0df468fd06a1316f24c00d5f47c2f5c9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/561436 Reviewed-by: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
|
Change https://go.dev/cl/561675 mentions this issue: |
This restores the concurrent walking that was removed in CL 508506, since that concurrency turns out to actually matter in practice (see golang/go#44863), but uses a different (and in my opinion simpler) concurrency pattern based on the one shown in my 2018 GopherCon talk (https://drive.google.com/file/d/1nPdvhB0PutEJzdCq5ms6UI58dp50fcAN/view, slide 114), and removes the arbitrary 4-goroutine minimum. On my machine this speeds up the benchmark from CL 561436 by a factor of around 3½. goos: linux goarch: amd64 pkg: golang.org/x/tools/internal/imports cpu: Intel(R) Xeon(R) CPU @ 2.20GHz │ before.txt │ after.txt │ │ sec/op │ sec/op vs base │ ModuleResolver_InitialScan-24 1728.0m ± 5% 505.2m ± 7% -70.76% (p=0.000 n=10) Fixes golang/go#65531. Updates golang/go#44863. Change-Id: I082bb3375f7775d55d130bf75ae71f53312aace1 Reviewed-on: https://go-review.googlesource.com/c/tools/+/561675 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Auto-Submit: Bryan Mills <[email protected]>
When using the gopls daemon, or with the multi-View workspaces that will be increasingly common following golang/go#57979, there is a lot of redundant work performed scanning the module cache. This CL eliminates that redundancy, by moving module cache information into the cache.Cache shared by all Sessions and Views. There should be effectively no change in behavior for gopls resulting from this CL. In ModuleResolver.scan, we still require that module cache roots are scanned. However, we no longer invalidate this scan in ModuleResolver.ClearForNewScan: re-scanning the module cache is the responsibility of a new ScanModuleCache function, which is independently scheduled. To enable this separation of refresh logic, a new refreshTimer type is extracted to encapsulate the refresh logic. For golang/go#44863 Change-Id: I333d55fca009be7984a514ed4abdc9a9fcafc08a Reviewed-on: https://go-review.googlesource.com/c/tools/+/559636 Reviewed-by: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
|
I believe this should be as fixed as it's going to get, for the time being. Shared sessions on a common remote will share their view of the module cache. There's a lot we can do to speed up the initial indexing of the module cache, but that is more significant engineering effort and best left for a holistic redesign of goimports. |
|
This is outstanding, thank you. Very much looking forward to giving this all a test! |
For correctness reasons, gopls uses a separate goimports cache per workspace folder. In some environments, warming up this cache can be costly (see also #42861 and #41969). Users of the gopls daemon will pay this warm-up cost for each new session, which can be disruptive for workflows based around short-lived sessions (e.g. opening vim to perform a quick edit).
It's probably possible to significantly mitigate this warm-up time by sharing the directory scan of the module cache across goimports processEnvs. From discussion with @heschik, we already have a
dirInfoCacheabstraction, and it probably only depends on (GOPATH,GOOS, andGOARCH). If this state were shared through the gopls cache, goimports would probably be quick for new vim sessions.This is following up on a discussion in Slack with @myitcv, @leitzler, and @mvdan.
CC @stamblerre
The text was updated successfully, but these errors were encountered: