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/goimports: performance issue with go1.11 in modules #27287

Closed
thatguystone opened this issue Aug 27, 2018 · 12 comments
Closed

x/tools/cmd/goimports: performance issue with go1.11 in modules #27287

thatguystone opened this issue Aug 27, 2018 · 12 comments

Comments

@thatguystone
Copy link

@thatguystone thatguystone commented Aug 27, 2018

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

go version go1.11 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="~/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="~/go"
GOPROXY=""
GORACE=""
GOROOT="~/.gimme/versions/go1.11.linux.amd64"
GOTMPDIR=""
GOTOOLDIR="~/.gimme/versions/go1.11.linux.amd64/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="~/appengine/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-build507619798=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Compile goimports with go1.10 and go1.11, then run on google.golang.org/appengine outside of $GOPATH. Full setup:

~ $ git clone --depth=1 https://github.com/golang/appengine.git
Cloning into 'appengine'...
remote: Counting objects: 211, done.
remote: Compressing objects: 100% (194/194), done.
remote: Total 211 (delta 12), reused 103 (delta 9), pack-reused 0
Receiving objects: 100% (211/211), 365.12 KiB | 4.68 MiB/s, done.
Resolving deltas: 100% (12/12), done.
~ $ cd go/src/golang.org/x/tools/cmd/goimports/
~/go/src/golang.org/x/tools/cmd/goimports $ git pull
Already up to date.
~/go/src/golang.org/x/tools/cmd/goimports $ go version
go version go1.10.3 linux/amd64
~/go/src/golang.org/x/tools/cmd/goimports $ go build
~/go/src/golang.org/x/tools/cmd/goimports $ mv goimports ~/appengine/goimports1.10
~/go/src/golang.org/x/tools/cmd/goimports $ eval "$(gimme 1.11)"
go version go1.11 linux/amd64
~/go/src/golang.org/x/tools/cmd/goimports $ go build
~/go/src/golang.org/x/tools/cmd/goimports $ mv goimports ~/appengine/goimports1.11
~/go/src/golang.org/x/tools/cmd/goimports $ cd ~/appengine/

What did you expect to see?

~/appengine $ time ./goimports1.10 -w .

real	0m0.788s
user	0m0.856s
sys	0m0.029s

What did you see instead?

~/appengine $ time ./goimports1.11 -w .

real	0m9.400s
user	0m12.023s
sys	0m4.576s

Outside of $GOPATH, goimports compiled with go1.11 runs much slower. Inside of $GOPATH, it's basically the same:

~/go/src/google.golang.org/appengine $ time goimports1.10 -w . && time goimports1.11 -w .

real	0m0.482s
user	0m0.505s
sys	0m0.020s

real	0m0.439s
user	0m0.464s
sys	0m0.013s
@gopherbot gopherbot added this to the Unreleased milestone Aug 27, 2018
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 30, 2018

@myitcv
Copy link
Member

@myitcv myitcv commented Aug 30, 2018

@tzneal
Copy link
Member

@tzneal tzneal commented Aug 31, 2018

This was caused by a fix to #26504 in f851253, with modules enabled it's exec'ing go list on each import to determine import path names. Temporary fix at http://golang.org/cl/132598 until goimports moves to go/packages.

@mitranim
Copy link

@mitranim mitranim commented Sep 12, 2018

@tzneal Until this change is merged, could you advise how to pull it, for folks unfamiliar with Gerrit / googlesource?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 12, 2018

@mitranim Click on the Download link in the middle of the screen on the right to get three different ways to fetch the patch.

@bcmills bcmills added the modules label Sep 13, 2018
jeremyschlatter added a commit to jeremyschlatter/go-ethereum that referenced this issue Sep 26, 2018
In projects with large GOPATHs, goimports can take a long time to run.

goimports has also not been updated for Go 1.11 modules:

golang/go#27287
golang/go#27865
golang/go#24661

Instead of using goimports to insert import statements, this commit
generates imports directly in the Go template.
jeremyschlatter added a commit to jeremyschlatter/go-ethereum that referenced this issue Sep 26, 2018
In projects with large GOPATHs, goimports can take a long time to run.

goimports has also not been updated for Go 1.11 modules:

golang/go#27287
golang/go#27865
golang/go#24661

Instead of using goimports to insert import statements, this commit
generates imports directly in the Go template.
@math2001
Copy link

@math2001 math2001 commented Sep 30, 2018

until goimports moves to go/packages.

When will that be?

@myitcv
Copy link
Member

@myitcv myitcv commented Oct 7, 2018

@math2001 this work is in progress. For progress updates this is the correct tracking issue (#24661 is the umbrella tracking issue for "all" tools). You might also be interested in following the fortnightly update calls we have as part of the https://groups.google.com/forum/#!forum/golang-tools group.

@ianthehat
Copy link

@ianthehat ianthehat commented Oct 16, 2018

Progress in https://golang.org/cl/142697
Mostly working, but not ready to submit.

@konstlepa
Copy link

@konstlepa konstlepa commented Oct 17, 2018

Progress in https://golang.org/cl/142697
Mostly working, but not ready to submit.

I tried https://golang.org./cl/128362. It works only if a working directory or its parent contains go.mod. Look imports/dirs.go in the patchset for details.

I use the next script:

#!/bin/bash

argc=$#
argv=($@)

for (( j=0; j<argc; j++ )); do
    if [ ${argv[j]} == "-srcdir" ]; then
            cd $(dirname ${argv[j+1]})
            break
    fi
done

goimports.bin $*
@tschellenbach
Copy link

@tschellenbach tschellenbach commented Nov 26, 2018

Is this fix going to be included in 1.12?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 26, 2018

Is this fix going to be included in 1.12?

goimports isn't part of a release.

@heschik
Copy link
Contributor

@heschik heschik commented Dec 13, 2018

goimports at tip now uses go/packages for modules. It won't be as fast as non-module mode, since it still has to shell out to go list, but it should be usable. Please file (new) bugs if you have problems.

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
You can’t perform that action at this time.