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

Open
findleyr opened this issue Mar 8, 2021 · 4 comments
Open
Labels
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr 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 this to the Unreleased milestone Mar 8, 2021
@findleyr
Copy link
Contributor Author

@findleyr 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

@stamblerre stamblerre commented Mar 8, 2021

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 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 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: v1.0.0 Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants