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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: missing fields / methods in completion results #53992

Closed
findleyr opened this issue Jul 21, 2022 · 11 comments
Closed

x/tools/gopls: missing fields / methods in completion results #53992

findleyr opened this issue Jul 21, 2022 · 11 comments
Assignees
@findleyr
Labels
FrozenDueToAge gopls/performance Issues related to gopls performance (CPU, memory, etc). gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.12.3

Comments

@findleyr
Copy link
Member

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 This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, 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
Member Author

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@findleyr
Copy link
Member Author

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)

@findleyr findleyr mentioned this issue Aug 1, 2022
Closed
gopherbot pushed a commit to golang/tools that referenced this issue Aug 4, 2022
@findleyr
gopls/internal/regtest/bench: refactor and improve benchmarks
87f47bb 
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
@findleyr
gopls/internal/regtest/bench: add a test for completion following edits
06d96ee 
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
Contributor

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

gopherbot pushed a commit to golang/tools that referenced this issue Aug 15, 2022
@findleyr
internal/lsp/completion: don't use Type.String for checking identity
987de34 
Completion is very performance sensitive, and building a string to check
for *testing.F has a significant cost.

StructCompletion-8         20.7ms ±14%  16.8ms ± 1%  -18.59%  (p=0.000 n=10+10)
ImportCompletion-8         1.36ms ± 5%  1.05ms ±18%  -22.55%  (p=0.000 n=9+10)
SliceCompletion-8          23.5ms ± 2%  19.3ms ±18%  -17.85%  (p=0.000 n=7+10)
FuncDeepCompletion-8       17.6ms ± 2%  15.5ms ± 2%  -11.82%  (p=0.000 n=8+8)
CompletionFollowingEdit-8  81.2ms ± 8%  74.2ms ± 5%   -8.60%  (p=0.000 n=9+9)

For golang/go#53992
For golang/go#53798

Change-Id: Ia138cbadce142a424caabe8259bda05bcc536055
Reviewed-on: https://go-review.googlesource.com/c/tools/+/422906
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/423935 mentions this issue: go/types: optimize instance lookup in LookupFieldOrMethod

@findleyr findleyr modified the milestones: gopls/v0.10.0, gopls/v0.10.1 Oct 11, 2022
@findleyr findleyr added the gopls/performance Issues related to gopls performance (CPU, memory, etc). label Oct 11, 2022
@findleyr findleyr assigned pjweinb and unassigned findleyr Oct 12, 2022
gopherbot pushed a commit that referenced this issue Oct 13, 2022
@findleyr
go/types, types2: optimize instance lookup in LookupFieldOrMethod
ddd8bc1 
LookupFieldOrMethod appears as a hotspot when benchmarking gopls'
auto-completion. In particular, instanceLookup.add was allocating in the
common case of structs with no embedding.

This is easily fixed, by using a small array in front of the map inside
of instanceLookup. Do this, and additionally add a microbenchmark.

The benchmark improvement is significant:

name                    old time/op    new time/op    delta
LookupFieldOrMethod-12     388µs ± 6%     154µs ± 3%  -60.36%  (p=0.000 n=10+10)

name                    old alloc/op   new alloc/op   delta
LookupFieldOrMethod-12     152kB ± 0%       2kB ± 0%  -98.77%  (p=0.000 n=9+10)

name                    old allocs/op  new allocs/op  delta
LookupFieldOrMethod-12     1.41k ± 0%     0.07k ± 0%  -95.38%  (p=0.000 n=10+10)

It should also be noted that instanceLookup is used elsewhere, in
particular by validType. In those contexts, the scope is not just the
current type but the entire package, and so the newly added buffer is
likely to simply cause extra Identical checks. Nevertheless, those
checks are cheap, and on balance the improved LookupFieldOrMethod
performance leads overall to improved type-checking performance.
Standard library benchmark results varied by package, but type checking
speed for many packages improved by ~5%, with allocations improved by
~10%. If this weren't the case we could let the caller control the
buffer size, but that optimization doesn't seem necessary at this time.

For example:

Check/http/funcbodies/noinfo-12            71.5ms ± 4%    67.3ms ± 2%   -5.90%  (p=0.000 n=20+20)
Check/http/funcbodies/noinfo-12              244k ± 0%      219k ± 0%  -10.36%  (p=0.000 n=19+19)

Updates #53992

Change-Id: I10b6deb3819ab562dbbe1913f12b977cf956dd50
Reviewed-on: https://go-review.googlesource.com/c/go/+/423935
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
@findleyr findleyr modified the milestones: gopls/v0.10.1, gopls/v0.10.2 Nov 1, 2022
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
@findleyr @romaindoumenc
go/types, types2: optimize instance lookup in LookupFieldOrMethod
9d80f7b 
LookupFieldOrMethod appears as a hotspot when benchmarking gopls'
auto-completion. In particular, instanceLookup.add was allocating in the
common case of structs with no embedding.

This is easily fixed, by using a small array in front of the map inside
of instanceLookup. Do this, and additionally add a microbenchmark.

The benchmark improvement is significant:

name                    old time/op    new time/op    delta
LookupFieldOrMethod-12     388µs ± 6%     154µs ± 3%  -60.36%  (p=0.000 n=10+10)

name                    old alloc/op   new alloc/op   delta
LookupFieldOrMethod-12     152kB ± 0%       2kB ± 0%  -98.77%  (p=0.000 n=9+10)

name                    old allocs/op  new allocs/op  delta
LookupFieldOrMethod-12     1.41k ± 0%     0.07k ± 0%  -95.38%  (p=0.000 n=10+10)

It should also be noted that instanceLookup is used elsewhere, in
particular by validType. In those contexts, the scope is not just the
current type but the entire package, and so the newly added buffer is
likely to simply cause extra Identical checks. Nevertheless, those
checks are cheap, and on balance the improved LookupFieldOrMethod
performance leads overall to improved type-checking performance.
Standard library benchmark results varied by package, but type checking
speed for many packages improved by ~5%, with allocations improved by
~10%. If this weren't the case we could let the caller control the
buffer size, but that optimization doesn't seem necessary at this time.

For example:

Check/http/funcbodies/noinfo-12            71.5ms ± 4%    67.3ms ± 2%   -5.90%  (p=0.000 n=20+20)
Check/http/funcbodies/noinfo-12              244k ± 0%      219k ± 0%  -10.36%  (p=0.000 n=19+19)

Updates golang#53992

Change-Id: I10b6deb3819ab562dbbe1913f12b977cf956dd50
Reviewed-on: https://go-review.googlesource.com/c/go/+/423935
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
@bkessler-go bkessler-go assigned findleyr and unassigned pjweinb Dec 8, 2022
@findleyr findleyr modified the milestones: gopls/v0.11.0, gopls/v0.12.0 Dec 14, 2022
@findleyr findleyr mentioned this issue May 3, 2023
Closed
@findleyr findleyr modified the milestones: gopls/v0.12.0, gopls/v0.13.0 May 22, 2023
@findleyr findleyr modified the milestones: gopls/v0.13.0, gopls/v0.12.3 Jun 12, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/502976 mentions this issue: go/types/objectpath: memoize scope lookup in objectpath.Encoder

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/503015 mentions this issue: gopls/internal/lsp/cache: compute xrefs and methodsets asynchronously

gopherbot pushed a commit to golang/tools that referenced this issue Jun 13, 2023
@findleyr
go/types/objectpath: memoize scope lookup in objectpath.Encoder
c6c9830 
Profiling revealed that scope lookup itself was costly, so memoize the
objects themselves, not just their names.

Also update BenchmarkCompletionFollowingEdit to allow it to be run on
multiple repos, and add a test case for kubernetes.

For golang/go#53992

Change-Id: I17b1f39d8c356e8628610a544306686573a813a7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/502976
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Bypass: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/503316 mentions this issue: gopls/internal/lsp/cache: remove ITVs from shared import graph

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/503016 mentions this issue: gopls/internal/lsp/source/completion: ensuring completion completeness

gopherbot pushed a commit to golang/tools that referenced this issue Jun 14, 2023
@findleyr
gopls/internal/lsp/cache: compute xrefs and methodsets asynchronously
f394d45 
No need to wait on xrefs or methodsets while performing
latency-sensitive operations on packages.

For golang/go#53992

Change-Id: I9ea65269a8c1e604fb99ed8b25e14db79f179576
Reviewed-on: https://go-review.googlesource.com/c/tools/+/503015
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
@findleyr findleyr mentioned this issue Jun 20, 2023
Closed
gopherbot pushed a commit to golang/tools that referenced this issue Jun 21, 2023
@findleyr
gopls/internal/lsp/cache: remove ITVs from shared import graph
63624df 
The shared import graph exists to optimize type-checking of open
packages following a change. But we should only ever ask for ITVs when
type-checking the entire workspace (in the debounced call to
diagnoseSnapshot).

For golang/go#53992

Change-Id: Ie968d4b2a0476822999aa216c678bed370aaa67b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/503316
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
@findleyr findleyr changed the title x/tools/gopls: measure and audit completion performance x/tools/gopls: missing fields / methods in completion results Jun 22, 2023
@golang golang locked and limited conversation to collaborators Jun 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@findleyr findleyr

Labels
FrozenDueToAge gopls/performance Issues related to gopls performance (CPU, memory, etc). 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
Milestone
gopls/v0.12.3
Development

No branches or pull requests
4 participants
@pjweinb @gopherbot @muirdm @findleyr