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: progress notifications for more operations #71751

Open
angles-n-daemons opened this issue Feb 14, 2025 · 11 comments
Open

x/tools/gopls: progress notifications for more operations #71751

angles-n-daemons opened this issue Feb 14, 2025 · 11 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. gopls Issues related to the Go language server, gopls. help wanted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/backlog

Comments

@angles-n-daemons
Copy link

gopls version

Build info

command-line-arguments (unknown)
@
github.com/BurntSushi/toml@v1.4.1-0.20240526193622-a339e1f7089c h1:pxW6RcqyfI9/kWtOwnv/G+AzdKuy2ZrqINhenH4HyNs=
github.com/google/go-cmp@v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
github.com/kr/pretty@v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
github.com/kr/text@v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
github.com/rogpeppe/go-internal@v1.13.1 h1:KvO1DLK/DRN07sQ1LQKScxyZJuNnedQ5/wKSR38lUII=
github.com/ryboe/q@v1.0.24 h1:GgnsKqyULXRp6fw9gZOO3ZEsBHmsFVampM1TLWw9uvU=
golang.org/x/exp/typeparams@v0.0.0-20241210194714-1829a127f884 h1:1xaZTydL5Gsg78QharTwKfA9FY9CZ1VQj6D/AZEvHR0=
golang.org/x/mod@v0.23.0 h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM=
golang.org/x/sync@v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w=
golang.org/x/telemetry@v0.0.0-20241220003058-cc96b6e0d3d9 h1:L2k9GUV2TpQKVRGMjN94qfUMgUwOFimSQ6gipyJIjKw=
golang.org/x/text@v0.22.0 h1:bofq7m3/HAFvbF51jz3Q9wLg3jkvSPuiZu/pD1XwgtM=
golang.org/x/tools@v0.28.0 => ../
golang.org/x/tools/gopls@(devel)
golang.org/x/vuln@v1.1.3 h1:NPGnvPOTgnjBc9HTaUx+nj+EaUYxl5SJOWqaDYGaFYw=
honnef.co/go/tools@v0.5.1 h1:4bH5o3b5ZULQ4UrBmP+63W9r7qIkqJClEA9ko5YKx+I=
mvdan.cc/gofumpt@v0.7.0 h1:bg91ttqXmi9y2xawvkuMXyvAA/1ZGJqYAEGjXuP0JXU=
mvdan.cc/xurls/v2@v2.5.0 h1:lyBNOm8Wo71UknhUs4QTFUNNMyxy2JEIaKKo0RWOh+8=
go: go1.24.0

go env

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE=''
GOARCH='arm64'
GOARM64='v8.0'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/Users/briandillmann/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/briandillmann/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/8j/_m26x9551t9b2zjfxw0d86380000gp/T/go-build2964405487=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/Users/briandillmann/go/src/github.com/golang/tools/gopls/go.mod'
GOMODCACHE='/Users/briandillmann/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/briandillmann/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/briandillmann/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.darwin-arm64'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/briandillmann/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/briandillmann/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.0.darwin-arm64/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

Slowness can be reproduced against the CockroachDB repo, using goto implementation:

git clone git@github.com:cockroachdb/cockroach.git
git checkout 562d26b3c86fbc0fb5363ad2bfeab118c2e08c58
# if there's a better command to first index the codebase, so that latency can be observed on a warm gopls server, add it here.
gopls implementation ./pkg/kv/kvserver/split/decider.go:48:4

What did you see happen?

Goto Implementation can take a long time, even on a warm server.

What did you expect to see?

It's not that I think goto implementation should execute faster, but for operations which take a long time, I would expect progress notifications to be sent, so that there's an indicator that gopls is working on my request.

Original request here: https://gophers.slack.com/archives/CJZH85XCZ/p1739487184239879

Editor and settings

Editor is neovim. Lack of progress notifications on slow requests should be observable for all gopls integrations however.

Logs

No response
@angles-n-daemons angles-n-daemons added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Feb 14, 2025
@gopherbot gopherbot added this to the Unreleased milestone Feb 14, 2025
@findleyr
Copy link
Member

Thanks. I think it is generally reasonable to start progress notifications after a delay, for any operation that could take a long time. In this case, we may want to look into what's taking so long, but we'd welcome a contribution similar to the PR you shared in slack.

@findleyr findleyr modified the milestones: Unreleased, gopls/backlog Feb 14, 2025
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the FeatureRequest Issues asking for a new feature that does not need a proposal. label Feb 14, 2025
@angles-n-daemons
Copy link
Author

Image

Profile from slow implementation call

@angles-n-daemons
Copy link
Author

implementation.txt

^ the above is a .prof file, (wouldn't let me drop it in as .prof)

@angles-n-daemons
Copy link
Author

snapshot.MethodSets is the line which required the most time. It took 50+ seconds, roughly 98% of the call time.

https://github.com/golang/tools/blob/master/gopls/internal/golang/implementation.go#L175

@findleyr
Copy link
Member

Thank you, @angles-n-daemons. Can you run gopls stats -anon from your workspace and share the output? It contains some information about the dimensions of your repo.

@angles-n-daemons
Copy link
Author 

Initializing workspace...


done (53.928417083s)
Gathering bug reports...      done (1.872516667s)
Querying memstats...          done (739.390333ms)
Querying workspace stats...   done (74.438ms)
Collecting directory info...  done (4.0448345s)
{
  "DirStats": {
    "Files": 215525,
    "TestdataFiles": 4961,
    "GoFiles": 8250,
    "ModFiles": 6,
    "Dirs": 32482
  },
  "GOARCH": "arm64",
  "GOOS": "darwin",
  "GOPACKAGESDRIVER": "",
  "GOPLSCACHE": "",
  "GoVersion": "go1.24.0",
  "GoplsVersion": "(unknown)",
  "InitialWorkspaceLoadDuration": "53.928417083s",
  "MemStats": {
    "HeapAlloc": 1645693560,
    "HeapInUse": 2872909824,
    "TotalAlloc": 31967301072
  },
  "WorkspaceStats": {
    "Files": {
      "Total": 20273,
      "Largest": 4950165,
      "Errs": 0
    },
    "Views": [
      {
        "GoCommandVersion": "go1.22.8",
        "AllPackages": {
          "Packages": 8717,
          "LargestPackage": 827,
          "CompiledGoFiles": 76634,
          "Modules": 362
        },
        "WorkspacePackages": {
          "Packages": 2037,
          "LargestPackage": 348,
          "CompiledGoFiles": 11977,
          "Modules": 1
        },
        "Diagnostics": 10
      }
    ]
  }
}

@angles-n-daemons
Copy link
Author

angles-n-daemons commented Feb 25, 2025 •
edited
Loading

This is for the CockroachDB repo (edit, this command was run from within the repo):

https://github.com/cockroachdb/cockroach

@angles-n-daemons
Copy link
Author

Seems like at the highest level, we'd need to emit progress reports at the Snapshot.forEachPackage function.

I can pass a reporter down from server.Implemetation -> golang.Implementation -> golang.implementation -> Snapshot.MethodSets -> Snapshot.forEachPackage but that would be changing a lot of signatures.

I could also embed a reporter on the Context, but there's a lack of type safety there.

Do you have any thoughts on this?

@findleyr
Copy link
Member

Sorry for the delayed response.

Let's start by

  1. Passing a (possibly nil) *progress.Tracker through to Snapshot.MethodSets.
  2. Encapsulating the "slow operation" logic of Snapshot.Analyze, perhaps as a method on the progress.Tracker.

It's not the type safety that makes me hesitant to stash on the Context--I'm sure we'd catch any bug related to mismatching context. Rather, it's the fact the presence of a tracker affects the behavior of the operation. It would be too easy to pass the context to some other suboperation of Implementation that ends up in forEachPackage, resulting in (for example) confusingly duplicated progress bars.

@angles-n-daemons
Copy link
Author

Perfect, yeah that makes sense - I'll try to have some draft out by this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. gopls Issues related to the Go language server, gopls. help wanted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/backlog
Development

No branches or pull requests
4 participants
@angles-n-daemons @gopherbot @findleyr @gabyhelp