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
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@thatguystone
Copy link

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 FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 30, 2018
@FiloSottile
Copy link
Contributor

/cc @bradfitz @josharian

@myitcv
Copy link
Member

myitcv commented Aug 30, 2018

/cc @ianthehat

@tzneal
Copy link
Member

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

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

@math2001
Copy link

until goimports moves to go/packages.

When will that be?

@myitcv
Copy link
Member

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

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

@konstlepa
Copy link

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

Is this fix going to be included in 1.12?

@bradfitz
Copy link
Contributor

Is this fix going to be included in 1.12?

goimports isn't part of a release.

@heschi
Copy link
Contributor

heschi 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.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests