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: add ability to avoid scanning an entire workspace #53278

Closed
euroelessar opened this issue Jun 7, 2022 · 11 comments
Closed

x/tools/gopls: add ability to avoid scanning an entire workspace #53278

euroelessar opened this issue Jun 7, 2022 · 11 comments
Labels
FrozenDueToAge gopls/workspace Issues related to support for modules or multi-module workspaces. 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
gopls/v0.9.0

Comments

@euroelessar
Copy link

gopls version

Build info
----------
golang.org/x/tools/gopls master
    golang.org/x/tools/gopls@(devel)
    github.com/BurntSushi/toml@v1.0.0 h1:dtDWrepsVPfW9H/4y7dDgFc2MBUSeJhlaDtK13CxFlU=
    github.com/google/go-cmp@v0.5.7 h1:81/ik6ipDQS2aGcBfIN5dHDB36BwrStyeAQquSYCV4o=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/exp/typeparams@v0.0.0-20220218215828-6cf2b201936e h1:qyrTQ++p1afMkO4DPEeLGq/3oTsdlvdH4vqZUBWzUKM=
    golang.org/x/mod@v0.6.0-dev.0.20220419223038-86c51ed26bb4 h1:6zppjxzCulZykYSLyVDYbneBfbaBIQPYMevg0bEwv2s=
    golang.org/x/sync@v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ=
    golang.org/x/sys@v0.0.0-20220209214540-3681064d5158 h1:rm+CHSpPEEW2IsXUib1ThaHIjuBVZjxNgSKmBLFfD4c=
    golang.org/x/text@v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk=
    golang.org/x/tools@(devel)
    golang.org/x/vuln@v0.0.0-20220503210553-a5481fb0c8be h1:jokAF1mfylAi1iTQx7C44B7vyXUcSEMw8eDv0PzNu8s=
    honnef.co/go/tools@v0.3.0 h1:2LdYUZ7CIxnYgskbUZfY7FPggmqnh6shBqfWa8Tn3XU=
    mvdan.cc/gofumpt@v0.3.0 h1:kTojdZo9AcEYbQYhGuLf/zszYthRdhDNDUi2JKTxas4=
    mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc=
go: go1.18.3

go env

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/elessar/.cache/go-build"
GOENV="/home/elessar/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/elessar/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/elessar/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/elessar/golang-runtime/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/elessar/golang-runtime/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/elessar/go/src/golang.org/x/tools/gopls/go.mod"
GOWORK="/home/elessar/go/src/go.work"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2549185785=/tmp/go-build -gno-record-gcc-switches"

What did you do?

We've recently made an attempt to add go.work file to our monorepo.

What did you expect to see?

No performance/behavior degradation, only behavior ones (like fetching 3rd party modules via go mod instead of relying on their presence in GOPATH).

What did you see instead?

Adding go.work file has drastically changed gopls behavior compared to the state before:

  1. Start time (initialization) has increased from couple of seconds to two minutes
  2. Memory usage at startup has increased from 200MB to 20GB
  3. textDocument/didChange per-call latency has increased from 2ms to 250ms (given a faulty batching in vscode-go it leads to multi-second delay between finishing typing & getting completion suggestions as it looks like every individual character change is sent via separate event)

Based on experiments it looks like before adding go.work file gopls did not make an attempt to scan and parse an entire GOPATH codebase, instead it was parsing only transitive dependencies of opened go files.

While I understand that it potentially was not an intended gopls behavior, but are you open to return it via an additional opt-in option? Without that gopls in current state is not usable on a repository of the given size.
Based on my limited knowledge it can be achieved by short-circuting (*snapshot).loadWorkspace logic.

Note, I've also tried to set memoryMode to DegradeClosed and while it reduces memory usage considerably (from 20GB to 2GB) it unfortunately does not improve textDocument/didChange latency.

Somehow side-tracking, textDocument/didChange spends mostly all of its time in copying the snapshot, which runtime complexity (as was discussed in other issues) can likely be optimized by using persistent data structures. Are you open for changes in that area? If so, what channel can be used to discuss the details and potential avenues for it (like slack?)?

Editor and settings

VS Code
Version: 1.67.2 (Universal)
Commit: c3511e6c69bb39013c4a4b7b9566ec1ca73fc4d5
Date: 2022-05-17T18:20:57.384Z (3 wks ago)
Electron: 17.4.1
Chromium: 98.0.4758.141
Node.js: 16.13.0
V8: 9.8.177.13-electron.0
OS: Darwin x64 21.5.0

Logs
@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 Jun 7, 2022
@gopherbot gopherbot added this to the Unreleased milestone Jun 7, 2022
@jamalc jamalc added the gopls/workspace Issues related to support for modules or multi-module workspaces. label Jun 8, 2022
@jamalc jamalc modified the milestones: Unreleased, gopls/later Jun 8, 2022
@jamalc jamalc added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 8, 2022
@findleyr
Copy link
Contributor

findleyr commented Jun 8, 2022

Regarding didChange processing time, we're working on landing the stack in #45686 now (for the next release). This should help significantly, though there will still be a fair bit of time spent "inheriting", and unfortunately that cant be avoided without more significant changes to gopls' caching logic. We are going to make those changes, but it will take longer.

Assuming you've narrowed your workspace in the go.work, the real problem is that gopls is walking files unnecessary, and that's just a bug. We have a couple open issues related to this also in the v0.9.0 milestone.

Would you be willing to try out some experimental patches in your workspace?

@jamalc jamalc modified the milestones: gopls/later, gopls/v0.9.0 Jun 9, 2022
@euroelessar
Copy link
Author

Hi, yeah, I can try them out.

I've also made some experiments myself, one thing I've noticed is that a huge chunk of time is spent in copying goFiles and files maps between generations.
Moving them to persistent treap has reduced didChange time from ≈250ms to ≈150ms, while still preserving refcounter-based destruction of associated handles (references are tracked by treap itself).
As far I understand none of your changes do actually address this particular point.

@findleyr
Copy link
Contributor

findleyr commented Jun 9, 2022

@euroelessar https://go.dev/cl/410176 addresses that point, as a starting point. Sorry, that wasn't linked to the issue.

We also have optimizations for Inherit in the works.

@findleyr
Copy link
Contributor

findleyr commented Jun 9, 2022

Also: would you be willing to share your implementation? We'd certainly accept a contribution in this area.

@euroelessar
Copy link
Author

Sure, let me clean it up a bit, will send it later today

@findleyr
Copy link
Contributor

findleyr commented Jun 9, 2022

CC @adonovan

@findleyr
Copy link
Contributor

findleyr commented Jun 9, 2022

@euroelessar before you spend time cleaning it up, you can share what you have and we can confirm that it meets requirements. There are some non-obvious reasons why we use a reference counted cache, for example sharing results across multiple views / sessions. But there are of course other ways to achieve this.

@euroelessar
Copy link
Author

Please see golang/tools#382

@euroelessar
Copy link
Author

@findleyr does it satisfy the requirements, should I continue investment in the change & make it satisfy the code quality requirements? (like add documentation, fix code style, add typing, etc)

based on my experiments the combination of this change (extended to s.files & s.packages) + your metadata-related patches (ones which avoid copying the metadata graph in common case) + caching known dirs (https://go-review.googlesource.com/c/tools/+/411636) allow going from original 250ms per didChange call down to 8ms, which looks good enough

@findleyr
Copy link
Contributor

@euroelessar, first of all thanks for the time and thought you have put into this. I think there are some nice ideas in your CL, but I haven't had time to do a deep review.

I am away this next week. In the meantime, I defer to @adonovan for how to proceed (or we can simply delay a week).

@findleyr
Copy link
Contributor

findleyr commented Jul 1, 2022

The GOPATH scan should have been fixed by https://go.dev/cl/413689. I think the other optimizations mentioned here are either complete, or tracked by separate issues. Thanks again for your contributions!

@findleyr findleyr closed this as completed Jul 1, 2022
@golang golang locked and limited conversation to collaborators Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls/workspace Issues related to support for modules or multi-module workspaces. 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
Milestone
gopls/v0.9.0
Development

No branches or pull requests
4 participants
@euroelessar @gopherbot @jamalc @findleyr