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/cmd/goimports: (unmerged CL version) slow resolution with large module cache #28518

Closed
myitcv opened this issue Oct 31, 2018 · 6 comments
Closed

x/tools/cmd/goimports: (unmerged CL version) slow resolution with large module cache #28518

myitcv opened this issue Oct 31, 2018 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. ToolSpeed
Milestone
Unreleased

Comments

@myitcv
Copy link
Member

myitcv commented Oct 31, 2018 •
edited

NOTE: this issue relates to an unmerged CL.

What version of Go are you using (go version)?

go version go1.11.1 linux/amd64

goimports as of
git fetch https://go.googlesource.com/tools refs/changes/97/142697/6 && git checkout FETCH_HEAD

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/go-modules-by-example/.gopath"
GOPROXY=""
GORACE=""
GOROOT="/home/myitcv/gos"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/go-modules-by-example/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-build922624083=/tmp/go-build -gno-record-gcc-switches"

What did you do?

See repro script and output at: https://gist.github.com/myitcv/bbe8a91813e7a8fc5d2fc599013cf305

The general setup is:

  • Create a simple module that is missing an "os/exec" import (but otherwise would compile)
  • Run goimports and see that it resolves quickly
  • Fill the module cache with a load of stuff
  • Run goimports and see that it now resolves much more slowly

What did you expect to see?

Given the missing import is "os/exec", a fast goimports resolution should be possible irrespective of the state of my module cache.

What did you see instead?

A much slower goimports resolution with a large module cache: 7.5s vs 0.2s.

cc @heschik
@myitcv myitcv added ToolSpeed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 31, 2018
@gopherbot gopherbot added this to the Unreleased milestone Oct 31, 2018
@myitcv
Copy link
Member Author

myitcv commented Oct 31, 2018

Per discussions with @heschik offline, we've tracked down the issue.

Now the repro (with fix applied):

https://gist.github.com/myitcv/a9fcd3c7af7ef816711c83b300ea2a9d

has the second set of goimports runs at ~0.6s.

Hence I'll close this and follow up in https://go-review.googlesource.com/c/tools/+/142697/

@myitcv myitcv closed this as completed Oct 31, 2018
@heschi
Copy link
Contributor

heschi commented Oct 31, 2018

For posterity, the problem was that the scratch module's dependencies couldn't be resolved, but figuring that out took go list quite a while and some network access. Since it ended up failing, the result wasn't cached, making it slow every time. Setting GOPROXY=off or to $GOPATH/pkg/mod/cache prevented the network access and (for reasons unknown) allowed the build to happen.

@bcmills, is the last sentence above something you'd expect?

@heschi heschi mentioned this issue Oct 31, 2018
Open
@myitcv
Copy link
Member Author

myitcv commented Oct 31, 2018

the problem was that the scratch module's dependencies couldn't be resolved

Just to correct one point; the module in which we were running goimports had zero dependencies, other than stdlib.

Rather, I think the module cache was in a state that caused go list to fail/be slow/something. And it got into this state by me priming it with the result of trying to convert github.com/docker/docker to be a module.

But per https://go-review.googlesource.com/c/tools/+/142697/#message-c2037823b2f2751007f352553e57b89a555374f2, I don't think we should even have hit the path of checking the module cache.

@heschi
Copy link
Contributor

heschi commented Oct 31, 2018 •
edited

Yeah, sorry, in this case the "scratch module" I was referring to is an internal implementation detail of go/packages, not your module. I'm replying to the code review comment on the code review.

@myitcv
Copy link
Member Author

myitcv commented Oct 31, 2018

Ah I see, my bad.

@bcmills
Copy link
Contributor

bcmills commented Nov 8, 2018

Setting GOPROXY=off or to $GOPATH/pkg/mod/cache prevented the network access and (for reasons unknown) allowed the build to happen.

@bcmills, is the last sentence above something you'd expect?

That is unexpected. go list should fail in exactly the cases where go build does (and succeed when go build succeeds).

@golang golang locked and limited conversation to collaborators Nov 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. ToolSpeed
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@myitcv @bcmills @gopherbot @heschi