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

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

Comments

@muirdm
Copy link

@muirdm 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 label Jun 5, 2019
@muirdm
Copy link
Author

@muirdm 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 jirfag commented Jun 9, 2019

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
			}
		}
	}
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 10, 2019

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

@stamblerre stamblerre commented Jun 10, 2019

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

@stamblerre stamblerre closed this Jun 10, 2019
@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.
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.