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/imports: slow due to unnecessary vendor scans #22370

Open
glibsm opened this issue Oct 20, 2017 · 4 comments

Comments

@glibsm
Copy link

commented Oct 20, 2017

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

go version go1.9 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOOS="darwin"

What did you do?

Ran goimports -v main.go on a very small file missing an import.

What did you expect to see?

Expected the goimports command to run considerably faster, since on a
properly configured IDE this is ran on each file save.

What did you see instead?

Running goimports took much longer than expected. Running it several times
produces similar results, so it's not "cold".

goimports -v main.go  1.09s user 17.68s system 383% cpu 4.895 total
goimports -v main.go  1.06s user 17.12s system 445% cpu 4.081 total
goimports -v main.go  1.07s user 17.19s system 414% cpu 4.406 total

Debugging

A one line change was made to the x/tools/imports package to output each
directory being scanned:

diff --git a/imports/fix.go b/imports/fix.go
index e2460849..4eb5818b 100644
--- a/imports/fix.go
+++ b/imports/fix.go
@@ -548,6 +548,8 @@ func scanGoDirs(goRoot bool) {
                }
                testHookScanDir(srcDir)
                walkFn := func(path string, typ os.FileMode) error {
+                       log.Println(">>>", path)
+
                        dir := filepath.Dir(path)
                        if typ.IsRegular() {
                                if dir == srcDir {

It turns out that most of the directories being scanned were part of external
vendor directories, which could never be imported by the current package.

All of the output was captured into a log.txt file.

With my $GOPATH, these were the findings:

$ cat log.txt | grep ">>>" | wc -l
  353744
$ cat log.txt | grep ">>>" | grep "vendor" |  wc -l
  316463

In summary, there were 316k directories scanned that could have been ignored.

With a hacky local change to ignore external vendors during the scan, we saw a
significant performance improvement:

$GOPATH/src/golang.org/x/tools/cmd/goimports/goimports -v main.go   0.07s user
0.34s
system 428% cpu 0.097 total

cc @prashantv

@gopherbot gopherbot added this to the Unreleased milestone Oct 20, 2017

@glibsm

This comment has been minimized.

Copy link
Author

commented Oct 24, 2017

After digging in more I found that the function canUse gets called after all the candidates have been assembled (including hundreds of thousands of packages that can't be used).

Ideally, the recursive exploration of candidates under $GOPATH should short-circuit when encountering vendor and internal that are known not to be usable in the future. Recursing into them is a big waste of i/o and causes the bad performance numbers shown above.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 1, 2017

@glibsm, are you going to send a change?

@glibsm

This comment has been minimized.

Copy link
Author

commented Nov 9, 2017

@bradfitz Yes, I plan to do a CL this weekend. It will be my first, so I have to get all the setup done. I've had a local patch working to great effect for the past few weeks.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 14, 2017

Change https://golang.org/cl/77630 mentions this issue: tools/goimports: Skip directories that can't be imported

@gopherbot gopherbot added the Tools label Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.