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: optimize strategy for sending overlays to go/packages #32810

Open
muirdm opened this issue Jun 27, 2019 · 5 comments
Milestone

Comments

@muirdm
Copy link

@muirdm muirdm commented Jun 27, 2019

When opening files gopls still hits the expensive go/packages "overlay" case. gopls has logic to know a file is not a pure overlay after a DidSave event, but when starting up, Emacs only sends a bunch of DidOpen events. Perhaps we should track "onDisk" from the "os.Stat" we do in (*view).findFile instead of in DidSave?

/cc @stamblerre

@gopherbot gopherbot added this to the Unreleased milestone Jun 27, 2019
@gopherbot gopherbot added the gopls label Jun 27, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 28, 2019

I probably should have used a more descriptive name than onDisk - I had intended it to mean that the overlay file contents are equivalent to those on disk. I think we still need to send all of the overlays even if we know the file is on disk, in case the user has edited it without saving.

@muirdm

This comment has been minimized.

Copy link
Author

@muirdm muirdm commented Jun 28, 2019

Ah, ok. I misunderstood what onDisk meant.

Maybe we can read the files contents in DidOpen and set onDisk if they match?

It is also weird that having any unsaved file causes us to hit the go/packages overlay code when opening any new file. Most of the time we already know a file's package path, so it seems like go/packages could avoid doing the overlay "go list" calls if the package being loaded doesn't depend on the package with an overlay file?

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 28, 2019

I suppose we could add that check to solve the didOpen problem.

The issue with the overlays is that go/packages needs to match them to their packages to figure that part out. The way we use go/packages now, we basically ask it for all of the packages under a certain directory (that's the way file= works under the hood), so I think it still needs to do that. I wonder if go/packages could add logic not to look at overlays if it figured out the package that file belongs to and if that package doesn't have any go list errors - what do you think @matloob?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 28, 2019

Change https://golang.org/cl/184257 mentions this issue: internal/lsp: check file content on disk when opening

gopherbot pushed a commit to golang/tools that referenced this issue Jul 1, 2019
As per discussion on golang/go#32810, to avoid the `go list` storm caused by many
files being opened, we check if the file content opened is equivalent to
the content on disk. If so, we mark this file as "on disk" so that we
don't send it as an overlay to go/packages.

Updates golang/go#32810

Change-Id: I0a520cf91bbe933c9afb76d0842f5556ac4e5b28
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184257
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@muirdm

This comment has been minimized.

Copy link
Author

@muirdm muirdm commented Jul 3, 2019

I tested and the above change avoids the extra "go list" calls. Thanks!

Currently gopls only passes along the overlay file names to go/packages, and it seems like the implicit assumption in go/packages is we have no idea what package those files belong to. But most of the time gopls has already loaded the package and knows what package they belong to. For example for common case:

  1. User is editing company/foo/foo.go with unsaved changes (gopls already has already loaded package foo and knows path is company/foo)
  2. User opens company/bar/bar.go. If bar doesn't transitively depend on company/foo, it doesn't seem like go/packages needs to do any extra overlay work involving foo.go.
@stamblerre stamblerre changed the title x/tools/gopls: "go list" overlay storm when opening files x/tools/gopls: optimize strategy for sending overlays to go/packages Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.