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

Jump to bottom

x/tools/gopls: don't apply completion budget to shallow completions #41434

Closed
cbelsole opened this issue Sep 16, 2020 · 10 comments
Closed

x/tools/gopls: don't apply completion budget to shallow completions #41434

cbelsole opened this issue Sep 16, 2020 · 10 comments
Labels
FrozenDueToAge 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.5.3

Comments

@cbelsole
Copy link

What version of Go are you using (go version)?

$ go version go1.15.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes (0.5.0). Also tested in (0.4.4).

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/user/Library/Caches/go-build"
GOENV="/Users/user/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/user/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/user/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.15.1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/user/projects/tools/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xg/l6_pfk3x0_n5ltb8h3wd1k500000gn/T/go-build694027970=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

For large packages the completion was timing out. For example, typing fmt. produced no results. Using a local struct variable struct1. produced no results.

[Trace - 11:19:55.356 AM] Sending request 'textDocument/completion - (14)'.
Params: {"textDocument":{"uri":"file:///path/to/file.go"},"position":{"line":602,"character":5},"context":{"triggerKind":2,"triggerCharacter":"."}}
[Trace - 11:19:55.910 AM] Received response 'textDocument/completion - (14)' in 554ms.
Result: {"isIncomplete":false,"items":[]}

@heschik found the workaround. Adding:

"gopls": {
  "completionBudget": "0.5s"
}

to my vscode config gave the completion code more time to read the very large package. Now completion items are coming up.

There may be a bug here as:
Slack___gopls___Gophers

@heschik: I'm not sure we want shallow completions to be subject to the budget, like Muir said

(slack convo)

What did you expect to see?

I expected to see the fmt public package methods and the methods attached to struct1.

What did you see instead?

No completions came up.
@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 Sep 16, 2020
@gopherbot gopherbot added this to the Unreleased milestone Sep 16, 2020
@stamblerre stamblerre changed the title x/tools/gopls: Completion not bringing up results for large packages x/tools/gopls: don't apply completion budget to shallow completions Sep 16, 2020
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.5.1 Sep 16, 2020
@heschi
Copy link
Contributor

heschi commented Sep 25, 2020

@stamblerre, is the behavior change you made in golang.org/cl/227303 still the one we want? If not this would be an easy fix for @dandua98.

@dandua98
Copy link

This might actually be fixed (in 0.5.1) now that deep completion is bfs and is triggered at the end of completions. I think this might be one of those scenarios with very large packages where we end up going too deep and not finding anything. Would need to be able to reproduce this and verify though.

@heschi
Copy link
Contributor

heschi commented Sep 25, 2020

It may have been improved, but I think it's still an open question whether we want to ever give up on searching the first level.

@stamblerre
Copy link
Contributor

No, I think in that CL was a mistake on my part. If deep completions is more "isolated" now, it would be nice to only apply that timeout to deep completions, but to start the timer when the completion request starts. (I think I may have thought that that's what I was doing in that CL, but clearly I did it wrong...)

@gopherbot
Copy link

Change https://golang.org/cl/257974 mentions this issue: internal/lsp/source: run unimported completions after other deep completions.

@gopherbot
Copy link

Change https://golang.org/cl/258286 mentions this issue: internal/lsp/source: run deep completions before unimported completions

gopherbot pushed a commit to golang/tools that referenced this issue Sep 29, 2020
@dandua98 @stamblerre
[gopls-release-branch.0.5] internal/lsp/source: run deep completions …
1e1ba56 
…before unimported completions

Unimported completions are expensive and can use up a large portion of
completion budget just to find initial deep search candidates. This
change moves these expensive operations which search through the module
cache to after normal deep completions so we search through more useful
candidates first.

Fixes golang/go#41434
Fixes golang/go#41665

Change-Id: I6f3963f8c65c1a97833a35738d2e96420de2f6ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/257974
Run-TryBot: Danish Dua <danishdua@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Trust: Danish Dua <danishdua@google.com>
(cherry picked from commit c43c25c)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/258286
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Danish Dua <danishdua@google.com>
Trust: Rebecca Stambler <rstambler@golang.org>
@cbelsole
Copy link
Author

cbelsole commented Oct 8, 2020

Some feedback on this change. In our largest package I still need to keep "completionBudget": "0.5s" in the config in order to get results back.

@stamblerre stamblerre reopened this Oct 9, 2020
@stamblerre stamblerre added this to the gopls/v1.0.0 milestone Oct 9, 2020
@gopherbot
Copy link

Change https://golang.org/cl/262347 mentions this issue: internal/lsp/source/completion: use a minimum budget of 10ms for shallow completions

gopherbot pushed a commit to golang/tools that referenced this issue Oct 14, 2020
@dandua98
internal/lsp/source/completion: use a minimum budget of 10ms for shal…
1610a49 
…low completions

Type-checking can be very expensive for large packages and leave no
budget for shallow completions. This change adds a minimum budget of
10ms so we show the user at least some shallow suggestions.

Updates golang/go#41434

Change-Id: If2ef59c3fbdcfa2ebaabb21256cf606a4f9c14d6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/262347
Trust: Danish Dua <danishdua@google.com>
Run-TryBot: Danish Dua <danishdua@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@cbelsole
Copy link
Author

Tested this in golang.org/x/tools/gopls v0.5.2 and I am now getting completions in our largest package without using "completionBudget": "0.5s".

@stamblerre
Copy link
Contributor

Glad to hear it! I'll close this issue then.

@golang golang locked and limited conversation to collaborators Oct 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge 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.5.3
Development

No branches or pull requests
5 participants
@cbelsole @stamblerre @gopherbot @heschi @dandua98