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: measure and audit completion performance #53992

Open
findleyr opened this issue Jul 21, 2022 · 6 comments
Open

x/tools/gopls: measure and audit completion performance #53992

findleyr opened this issue Jul 21, 2022 · 6 comments
Labels
gopls Tools
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Jul 21, 2022

While working in x/tools recently, I observed that I was sometimes missing struct fields in completion results. This should never be the case.

It turns out these results are missing due to completion budget being exhausted. Setting "completionBudget" to "1s" worked. Bisecting, I discovered that prior to https://go.dev/cl/347563 I generally (though not always) got the results I expected.

Clearly our regression testing is not catching regressions in completion performance, and the default completion budget of 100ms can lead to significantly degraded results.

We need to:

  1. Investigate why struct fields could ever be missed in completion results.
  2. Evaluate our current completion benchmarks, improve them if necessary, and hook them into performance monitoring (see also x/tools,x/build: performance monitoring for gopls #53538).
  3. Look for low-hanging improvements to completion performance (there must be some, for example the performance hit of https://go.dev/cl/347563 should be easy to mitigate).
@findleyr findleyr added this to the gopls/v0.9.2 milestone Jul 21, 2022
@gopherbot gopherbot added Tools gopls labels Jul 21, 2022
@muirdm
Copy link

muirdm commented Jul 23, 2022

I think this return is likely the problem. Even "shallow" completion candidates still go through the deep search stuff, so that return could stop before even depth=0 candidates have been processed.

The easy fix would be to finish processing the current queue before returning. Or maybe only finish the current queue if it is depth=0.

@findleyr
Copy link
Contributor Author

findleyr commented Jul 25, 2022

Thanks @muirdm! We should guarantee that depth=0 stuff completes, in addition to fixing the performance regression.

@gopherbot
Copy link

gopherbot commented Jul 29, 2022

Change https://go.dev/cl/419988 mentions this issue: gopls/internal/regtest/bench: refactor and improve benchmarks

@gopherbot
Copy link

gopherbot commented Jul 29, 2022

Change https://go.dev/cl/420220 mentions this issue: gopls/internal/regtest/bench: add a test for completion following edits

@findleyr
Copy link
Contributor Author

findleyr commented Jul 29, 2022

Oh my goodness, a huge chunk of time is spent in go/types.missingMethodReason, via types.AssignableTo and types.ConvertibleTo, which is totally wasted work...

Will file an issue if one does not already exist... (later)

gopherbot pushed a commit to golang/tools that referenced this issue Aug 4, 2022
Significantly refactor the gopls benchmarks to turn them into proper
benchmarks, eliminate the need for passing flags, and allow running them
on external gopls processes so that they may be used to test older gopls
versions.

Doing this required decoupling the benchmarks themselves from the
regtest.Runner. Instead, they just create their own regtest.Env to use
for scripting operations. In order to facilitate this, I tried to
redefine Env as a convenience wrapper around other primitives.

By using a separate environment setup for benchmarks, I was able to
eliminate a lot of regtest.Options that existed only to prevent the
regtest runner from adding instrumentation that would affect
benchmarking. This also helped clean up Runner.Run somewhat, though it
is still too complicated.

Also eliminate the unused AnyDiagnosticAtCurrentVersion, and make a few
other TODOs about future cleanup.

For golang/go#53992
For golang/go#53538

Change-Id: Idbf923178d4256900c3c05bc8999c0c9839a3c07
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419988
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@findleyr findleyr modified the milestones: gopls/v0.9.2, gopls/v0.10.0 Aug 4, 2022
gopherbot pushed a commit to golang/tools that referenced this issue Aug 5, 2022
For golang/go#53992

Change-Id: Ia1f1e27663992707eef9226273b152117ee977ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420220
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
@gopherbot
Copy link

gopherbot commented Aug 13, 2022

Change https://go.dev/cl/422906 mentions this issue: internal/lsp/completion: don't use Type.String for checking identity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Tools
Projects
None yet
Development

No branches or pull requests

3 participants