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

Open
myitcv opened this issue Sep 29, 2020 · 4 comments
Open

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

myitcv opened this issue Sep 29, 2020 · 4 comments

Comments

@myitcv
Copy link
Member

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

@gopherbot gopherbot added the Tools label Sep 29, 2020
@gopherbot gopherbot added this to the Unreleased milestone Sep 29, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 29, 2020

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 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.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.