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.

Sign up for GitHub

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/cmd/gopls: overlays cause go/packages to perform giant "go list" query #32457

Closed
muirdm opened this issue Jun 5, 2019 · 4 comments
Closed

x/tools/cmd/gopls: overlays cause go/packages to perform giant "go list" query #32457

muirdm opened this issue Jun 5, 2019 · 4 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls.
Milestone
Unreleased

Comments

@muirdm
Copy link

muirdm commented Jun 5, 2019

I had ~60 go files open in gopls. None of them had any unsaved changes (i.e. bytes on disk matched bytes in gopls memory). I noticed when opening a new file in my editor gopls was taking a long time. I tracked it down to the overlay logic in go/packages.

Here is roughly what happened.

  1. I opened file foo.go in a previously unopened package.
  2. gopls does a go/packages file=foo.go query and passes an overlay that includes all 60 files.
  3. In processGolistOverlay() it appends a new package to response.Packages for each overlay file's package
  4. It marks all imports of response.Packages in needPkgsSet
  5. addNeededOverlayPackages() performs a "go list" call for all items in needPkgs, which takes 3 seconds in my case.

I don't fully understand the package overlay stuff, but here are my observations:

  • it doesn't seem like we need to treat "saved" files as overlays
  • even if they are "real" overlays, go/packages seems to be doing too much work when querying only for foo.go (i.e. most overlay packages are not related to foo.go's package, so why do we list them and all their dependencies?)
  • the overlay work is redone every time gopls invokes go/packages to look up a new file even if none of the overlays have changed

/cc @matloob @stamblerre
@gopherbot gopherbot added this to the Unreleased milestone Jun 5, 2019
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Jun 5, 2019
@muirdm
Copy link
Author

muirdm commented Jun 8, 2019

I'd be happy to work on this if I could get some direction on the expected behavior in this case.

@jirfag
Copy link

jirfag commented Jun 9, 2019 •
edited

The similar issue reproduces in golangci-lint: after updating x/tools from 685fecacd0a0 to 755ce86c7629 go/packages started loading dependencies when not needed.
As I understand the regression was introduced in this overlays commit.
And this code adds all dependencies into needPkgsSet and after this to needPkgs, which are used to run go list on them:

	// Do another pass now that new packages have been created to determine the
	// set of missing packages.
	for _, pkg := range response.Packages {
		for _, imp := range pkg.Imports {
			pkgPath := toPkgPath(imp.ID)
			if _, ok := havePkgs[pkgPath]; !ok {
				needPkgsSet[pkgPath] = true
			}
		}
	}

@jirfag jirfag mentioned this issue Jun 9, 2019
Merged
@gopherbot
Copy link

Change https://golang.org/cl/181123 mentions this issue: internal/lsp: add an implementation for textDocument/didSave

gopherbot pushed a commit to golang/tools that referenced this issue Jun 10, 2019
@stamblerre
internal/lsp: add an implementation for textDocument/didSave
e6ce0dd 
This change marks the overlay for a saved file as "on disk".
This will reduce the number of overlays we provide to go/packages, which
can be expensive.

Updates golang/go#31796, golang/go#32457

Change-Id: I8e69503ab80bba29caf4e42491d87e643bf17f1a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181123
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
@stamblerre stamblerre mentioned this issue Jun 10, 2019
Closed
@stamblerre
Copy link
Contributor

@muirrn: The change above should have reduced the overlays sent to go/packages to only the files that have not been saved.

I filed #32538 so that we can decide how to handle this in go/packages.

@golang golang locked and limited conversation to collaborators Jun 9, 2020
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.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@jirfag @stamblerre @gopherbot @muirdm