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

Closed
muirdm opened this issue Jun 27, 2019 · 6 comments
Closed

x/tools/gopls: optimize strategy for sending overlays to go/packages #32810

muirdm opened this issue Jun 27, 2019 · 6 comments
Labels
FrozenDueToAge 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.4.2

Comments

@muirdm
Copy link

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 Issues related to the Go language server, gopls. label Jun 27, 2019
@stamblerre
Copy link
Contributor

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
Copy link
Author

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

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
Copy link

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
@stamblerre
internal/lsp: check file content on disk when opening
e47c3d9 
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
Copy link
Author

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
@stamblerre stamblerre added help wanted Suggested Issues that may be good for new contributors looking for work to do. and removed Suggested Issues that may be good for new contributors looking for work to do. labels Aug 8, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Dec 4, 2019
@stamblerre stamblerre modified the milestones: gopls unplanned, gopls/v1.0.0 Jan 29, 2020
@stamblerre stamblerre added 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. and removed help wanted labels Mar 12, 2020
@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.5.0 May 21, 2020
@stamblerre
Copy link
Contributor

I don't think there are any further optimizations we can make here. It would be too much complexity to strategically not send overlays to go/packages.

@stamblerre stamblerre modified the milestones: gopls/v0.5.0, gopls/v0.4.2 Jun 24, 2020
@golang golang locked and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge 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.4.2
Development

No branches or pull requests
3 participants
@stamblerre @gopherbot @muirdm