x/tools/gopls: don't hang in allImportsFixes #59216
Comments
gopherbot
added
Tools
findleyr
added
release-blocker
okay-after-beta1
|
Change https://go.dev/cl/492679 mentions this issue: |
The current logic populates the process env *after* clearing, but clearing uses fields in the process env. To work around this, clearing is not performed on the first run. Chesterton's fence may apply: I don't really understand this, but don't see how it could be correct: if the clearing requires the environment, then the environment should be populated before clearing. For golang/go#59216 Change-Id: Ia0bc0de10284abf9853158e4f7e60de3d338083d Reviewed-on: https://go-review.googlesource.com/c/tools/+/492679 Reviewed-by: Alan Donovan <adonovan@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
|
Change https://go.dev/cl/494095 mentions this issue: |
Add some visibility into goimports operations, by instrumenting spans in top-level imports and gocommand operations. This may be the first time we instrument non-gopls code in this way, but it should be safe as other build targets (e.g. the goimports or gopackages commands) do not set a global exporter, and therefore the cost of event instrumentation should be minimal. For golang/go#59216 Change-Id: Id2f8fe05d6b61e96cdd2d41cc43b3d4c3cf39e21 Reviewed-on: https://go-review.googlesource.com/c/tools/+/494095 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com>
|
Just encountered this again, and the new instrumentation narrows this down to addExternalCandidates: |
|
While this is still a problem, it seems like it only hangs for perhaps 5-10 seconds. The real culprit of hanging in code actions was discovered, and fixed in https://go.dev/cl/496186. goimports isn't going to get rewritten for v0.12.0. Moving to v0.13.0, when hopefully we'll have time to unify goimports and unimported completion, and have a better handle on its execution. |
|
Just hit this again: |
|
I have this problem too when something goes wrong it hangs
where do I see that debug log? |
|
@ldemailly If you start gopls with the argument |
|
It happened to me today My only guess would be that I have lots of packages on GOPATH that take time to load, if that even is a thing. Would it make sense to add more instrumentation to narrow this down? |
Absolutely. Now that we have a few people tracking this, I think it makes sense to add more spans to see where the time is being spent (goimports code is not instrumented in the same way as the rest of gopls). @vikblom how frequently do you hit this? I seem to get it only ~once a month, so it has been hard to catch. |
|
Roughly once a week but it varies a lot. In my experience it happens more often when I'm working in many different repos and adding/removing dependencies. Probably just because that exercises this codepath more. |
|
for me it's more like a daily (several times today actually) it hangs each time I have a new import/compilation error |
|
Change https://go.dev/cl/544515 mentions this issue: |
While investigating a post-review comment on CL 538796, I observed that the importsState.SkipPathInScan func is building a directory filterer for every call. This can be very expensive, and may partially explain the performance problems encountered in golang/go#59216. Fix this, and update the test to catch bugs related to mixing up relpath vs abspath (a mistake I made during this CL). Updates golang/go#59216 Change-Id: I696429161ba039a380df1cf258edf78369acb398 Reviewed-on: https://go-review.googlesource.com/c/tools/+/544515 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com> Auto-Submit: Robert Findley <rfindley@google.com>
|
Just had allImportFixes take 34s, the longest I've seen by an order of magnitude, so this is definitely not fixed :( |
|
For anyone that stumbles across this issue. On my system this was made worse (or was caused) by an oversized GOPATH (check Running |
|
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
|
Change https://go.dev/cl/559458 mentions this issue: |
|
Change https://go.dev/cl/559635 mentions this issue: |
|
Change https://go.dev/cl/561235 mentions this issue: |
Switch the ModuleResolver from a lazy initialization model to a constructor, so that the resolver returned by ProcessEnv.GetResolver is ready to use. A constructor is easier to maintain as it involves less state, avoids redundant calls to init, and avoids bugs where the call to init is missing (such as was the case for the scoreImportPath method). It also lets the caller differentiate between a failure to construct the resolver and a resolver that only returns errors. Pragmatically, I'd like to move toward a model where the caller re-scans imports by asynchronously cloning, priming, and replacing the resolver, rather than blocking. This is a step in that direction. This change is not without risk, but it looks like all calls to ModuleResolver methods are preceded by a call to GetResolver, and errors from GetResolver are handled similarly errors from methods. There is some messiness resulting from this change, particularly in tests. These are noted inline, and can eventually be fixed by similarly avoiding lazy initialization of ProcessEnv. For golang/go#59216 Change-Id: I3b7206417f61a5697ed83e811c177f646afce3b2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/559635 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
This should be fixed at tip, since we now run goimports on the old cache until the new cache is ready. Please let me know if it's not fixed in your environment. |
Add some visibility into goimports operations, by instrumenting spans in top-level imports and gocommand operations. This may be the first time we instrument non-gopls code in this way, but it should be safe as other build targets (e.g. the goimports or gopackages commands) do not set a global exporter, and therefore the cost of event instrumentation should be minimal. For golang/go#59216 Change-Id: Id2f8fe05d6b61e96cdd2d41cc43b3d4c3cf39e21 Reviewed-on: https://go-review.googlesource.com/c/tools/+/494095 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com>
Thanks to the newly improved trace view, I have noticed that sometimes my gopls gets into a state where it hangs behind 'allImportFixes'. I expect this is the source of the often reported bug of 'waiting for code actions' from VS Code
Sample trace below, where AllImportsFixes took 3s for x/tools. (by comparison, type-checking all of x/tools from scratch takes ~3s).
I don't think we should ever wait a significant amount of time for allImportsFixes. goimports is already imprecise and/or nondeterministic, depending on the state of the module cache.
The text was updated successfully, but these errors were encountered: