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: don't apply completion budget to shallow completions #41434

Open
cbelsole opened this issue Sep 16, 2020 · 8 comments
Open

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

cbelsole opened this issue Sep 16, 2020 · 8 comments
Labels
Milestone

Comments

@cbelsole
Copy link

@cbelsole cbelsole commented Sep 16, 2020

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 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
@heschik
Copy link
Contributor

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

@dandua98 dandua98 commented Sep 25, 2020

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.

@heschik
Copy link
Contributor

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

@stamblerre stamblerre commented Sep 25, 2020

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

@gopherbot gopherbot commented Sep 29, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 29, 2020

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
…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 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
@stamblerre stamblerre added this to Bugs in gopls/v1.0.0 Oct 12, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 14, 2020

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
…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>
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
5 participants
You can’t perform that action at this time.