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: batch file modification notifications on the server side #41691

Closed
myitcv opened this issue Sep 29, 2020 · 5 comments
Closed

x/tools/gopls: batch file modification notifications on the server side #41691

myitcv opened this issue Sep 29, 2020 · 5 comments
Labels
FeatureRequest 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.7.1

Comments

@myitcv
Copy link
Member

myitcv commented Sep 29, 2020

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

$ go version
go version devel +aacbd7c3aa Thu Sep 24 09:15:20 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200928112810-42b62fc93869
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20200928112810-42b62fc93869

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=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
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=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/.vim/plugged/govim/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build941388356=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Working on https://github.com/play-with-docker/play-with-docker, I ran go mod vendor whilst I had govim open. vendor is about 53MB according to du. This resulted in 6711 calls to DidChangeWatchedFiles. All of these calls took just under 1s to execute, according to the govim logs.

But after this, gopls spent around 8 minutes at 100% CPU. I would guess it was processing the backlog of DidChangeWatchedFiles notifications.

However, a single cache invalidation after the last received DidChangeWatchedFiles notification would have sufficed.

So it would appear that the logic here for processing these notifications is not quite right. They do not contain files contents and so, to my understanding, can be entirely collapsed.

What did you expect to see?

gopls behaving normally.

What did you see instead?

gopls at 100%.

Log files: logs.zip

cc @stamblerre

FYI @leitzler
@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 Sep 29, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 29, 2020
@gopherbot gopherbot added this to the Unreleased milestone Sep 29, 2020
@stamblerre
Copy link
Contributor

However, a single cache invalidation after the last received DidChangeWatchedFiles notification would have sufficed.

It looks like the didChangeWatchedFiles notifications are not batched by the client (which other clients do). If they all came as one notification, they would have resulted in a single cache invalidation, but because they came individually, gopls processed them individually.

The way that gopls works right now is that it does not wait for other notifications to invalidate the cache. If there are good reasons to, we could consider it, but it seems much simpler to batch these notifications on the client side. Having gopls wait for all of the didChangeWatchedFiles notifications to arrive would require a lot of reworking of our invalidation model.

@stamblerre stamblerre removed this from the Unreleased milestone Sep 29, 2020
@myitcv
Copy link
Member Author

myitcv commented Sep 29, 2020

It looks like the didChangeWatchedFiles notifications are not batched by the client (which other clients do)

Thanks, I'm not sure how I had missed the fact these changes can be sent as a batch. I can make this change to fix things in the short term.

If there are good reasons to, we could consider it, but it seems much simpler to batch these notifications on the client side

I think that implementing a solution for this on the gopls side will allow for the most effective debouncing of notifications because the debounce implementation can leverage knowledge of the gopls implementation, invalidation model, etc. Clients will only ever be able to roughly approximate what "feels" right, whereas a gopls implementation could precisely collapse any notifications that are received whilst the handling of previous cache invalidations are "in flight".

Having gopls wait for all of the didChangeWatchedFiles notifications to arrive would require a lot of reworking of our invalidation model.

It wouldn't need to wait, because you could trigger a cache invalidation on the leading edge, collapsing any subsequent notifications you receive whilst the handling of that first invalidation is still in progress.

@myitcv myitcv mentioned this issue Sep 29, 2020
Open
@stamblerre stamblerre changed the title x/tools/gopls: rapid run of DidChangeWatchedFiles calls causes gopls to max out x/tools/gopls: batch file modification notifications on the server side Sep 29, 2020
@stamblerre stamblerre added FeatureRequest and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 29, 2020
@heschi
Copy link
Contributor

heschi commented Sep 29, 2020

I don't think it makes sense to worry about this until we have a concrete issue. I can't think of a scenario where a user would make thousands of changes spread out over a long period of time. Either they're going to make a bunch all at once with a branch change, or a few spread out file-by-file changes.

@myitcv
Copy link
Member Author

myitcv commented Sep 29, 2020

I can't think of a scenario where a user would make thousands of changes spread out over a long period of time.

Neither can I.

This issue is about my experience with go mod vendor, which is not guaranteed to be as "quick" as a branch change AFAICT.

Arguably you could say the LSP client (govim) did not behave as gopls expected here, but I think it's also fair to say that gopls could handle this better and in so doing would improve performance for all clients in this situation.

@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 2020
@findleyr findleyr self-assigned this Apr 9, 2021
@gopherbot
Copy link

Change https://golang.org/cl/309269 mentions this issue: internal/lsp: add a setting to batch didChangeWatchedFile notifications

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest 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.7.1
Development

No branches or pull requests
5 participants
@myitcv @stamblerre @gopherbot @heschi @findleyr