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: cannot choose the "right" value for completionBudget #63459

Open
myitcv opened this issue Oct 9, 2023 · 2 comments
Open

x/tools/gopls: cannot choose the "right" value for completionBudget #63459

myitcv opened this issue Oct 9, 2023 · 2 comments
Assignees
Labels
gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@myitcv
Copy link
Member

myitcv commented Oct 9, 2023

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

$ go version
go version go1.21.1 linux/arm64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.14.1-0.20231008020826-a3b5082fb05e
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20231008020826-a3b5082fb05e

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE='on'
GOARCH='arm64'
GOBIN=''
GOCACHE='/home/myitcv/.cache/go-build'
GOENV='/home/myitcv/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/myitcv/gostuff/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/myitcv/gostuff'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/myitcv/gos'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/myitcv/gos/pkg/tool/linux_arm64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3782877243=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Picking up the conversation from https://gophers.slack.com/archives/CRWSN9NCD/p1696429330059839

For the purposes of the discussion below, here are two example completions:

Example 1: a qualified identifier, unimported completion:

package main

func main() {
    load.Ins_
}

This should resolve to cuelang.org/go/cue/load.Instances.

Example 2: an unqualified completion:

package main

func main() {
    str_
}

My understanding based on the exchange with @findleyr on Slack of the current semantics around completionBudget is as follows:

  • completionBudget needs to be set to a "high" value in order that unimported completions (of the form taken in Example 1) don't time out, especially after a load from a cold cache but not limited to that. A large module cache in effect "forces" us to set a "high" value in this instance, because of the cost of (scanning and) searching the module cache.
  • completionBudget needs to be set to a "low" value in order that unqualified completions (of the form taken in Example 2) don't take a long time. Per @findleyr:

The problem is that this trivial completion is bundled together with a very non-trivial completion that scans the module cache.

The issue: if I choose a "high" value then Example 2 style completions can take ~200-250ms... which is very noticeable and annoying especially when the editor requests multiple in sequence as a completion candidate is refined. If I choose a "low" value, then I don't get unimported completions of Example 1 style in all situations.

I therefore cannot choose the "right" value.

What did you expect to see?

Being able to set a completionBudget that satisfies both cases, Examples 1 and 2.

What did you see instead?

The above.

Per @findleyr on Slack:

I think for starters we need to treat those two completions differently, so that you always get the trivial package name completion in identifier context, no matter how short your completionBudget is.


cc @findleyr

@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Oct 9, 2023
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Oct 9, 2023
@gopherbot gopherbot added this to the Unreleased milestone Oct 9, 2023
@findleyr findleyr modified the milestones: Unreleased, gopls/v0.15.0 Oct 12, 2023
@findleyr findleyr modified the milestones: gopls/v0.15.0, gopls/v0.16.0 Jan 17, 2024
@gopherbot
Copy link

Change https://go.dev/cl/560457 mentions this issue: gopls/internal/test/integration/bench: improve completion benchmarks

@gopherbot
Copy link

Change https://go.dev/cl/560458 mentions this issue: internal/imports: cache canonizalization

gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2024
Make the following improvements to the completion benchmarks:

- Run the "CompletionFollowingEdit" benchmarks with and without edits.
  Accordingly, rename it to just "BenchmarkCompletion", and shorten
  subtest names.
- BenchmarkCompletion is a cleaner way to specify completion tests.
  Deprecate other styles.
- Support running completion tests with a massive GOPATH, by passing
  -completion_gopath.
- Add a tools_unimportedselector test that seems to do a good job of
  exercising slow unimported completion. Other unimportedcompletion
  tests were short-circuited in one way or another (for example, because
  kubernetes uses vendoring, gopls doesn't scan GOPATH).

This will invalidate our benchmark dashboard, so we should merge this
change before subsequent CLs that optimize unimported completion.

For golang/go#63459

Change-Id: I9d7a06e3c1a7239b531ed8ff534f3174f4ac4ae8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560457
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@findleyr findleyr modified the milestones: gopls/v0.16.0, gopls/v0.17.0 Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

3 participants