Skip to content
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

x/tools/gopls: cold goimports cache causes slow code actions on first save #44863

Closed
findleyr opened this issue Mar 8, 2021 · 10 comments
Closed
Assignees
Labels
gopls/imports 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

@findleyr
Copy link
Contributor

findleyr commented Mar 8, 2021

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 dirInfoCache abstraction, and it probably only depends on (GOPATH, GOOS, and GOARCH). 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

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Mar 8, 2021
@gopherbot gopherbot added this to the Unreleased milestone Mar 8, 2021
@findleyr
Copy link
Contributor Author

findleyr commented Mar 8, 2021

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.

@stamblerre
Copy link
Contributor

After some discussion, we've agreed to try warming up the imports cache after the first didOpen of a Go file instead of after the first completion request.

@myitcv
Copy link
Member

myitcv commented Mar 9, 2021

Thanks very much for raising this, @findleyr

Maybe we could/should just warm up the cache immediately following initialization.

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 -remote is being used) then that would be the ultimate solution to my mind.

(I can imagine that when -remote is not being used that doing anything more aggressive will lead to problems in the other direction, i.e. potentially needless processing of the modules cache)

@findleyr
Copy link
Contributor Author

findleyr commented Mar 9, 2021

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).

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 findleyr changed the title x/tools/gopls: share goimports directory cache across views x/tools/gopls: cold goimports cache causes slow code actions on first save Mar 9, 2021
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Mar 10, 2021
@stamblerre stamblerre added this to To Do in gopls on-deck Mar 11, 2021
@stamblerre stamblerre removed this from To Do in gopls on-deck Jun 28, 2021
@findleyr findleyr modified the milestones: gopls/unplanned, gopls/later Jun 15, 2023
@findleyr findleyr modified the milestones: gopls/later, gopls/v0.15.0 Jul 31, 2023
@findleyr findleyr self-assigned this Jan 29, 2024
@gopherbot
Copy link

Change https://go.dev/cl/559496 mentions this issue: gopls/internal/cache/imports: simplify importsState invalidation

gopherbot pushed a commit to golang/tools that referenced this issue Jan 31, 2024
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/559636 mentions this issue: gopls/internal/cache: share goimports state for GOMODCACHE

@gopherbot
Copy link

Change https://go.dev/cl/561436 mentions this issue: internal/imports: add a benchmark for the initial mod cache scan

gopherbot pushed a commit to golang/tools that referenced this issue Feb 5, 2024
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 <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

Change https://go.dev/cl/561675 mentions this issue: internal/gopathwalk: walk directories concurrently

gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2024
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2024
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 <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@findleyr
Copy link
Contributor Author

findleyr commented Feb 6, 2024

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.

@findleyr findleyr closed this as completed Feb 6, 2024
@myitcv
Copy link
Member

myitcv commented Feb 6, 2024

This is outstanding, thank you. Very much looking forward to giving this all a test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls/imports gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants