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

Closed
glibsm opened this issue Oct 20, 2017 · 6 comments
Closed

x/tools/imports: slow due to unnecessary vendor scans #22370

glibsm opened this issue Oct 20, 2017 · 6 comments
Labels
FrozenDueToAge help wanted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@glibsm
Copy link

glibsm 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
Copy link
Author

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

bradfitz commented Nov 1, 2017

@glibsm, are you going to send a change?

@glibsm
Copy link
Author

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

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

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@heschi
Copy link
Contributor

heschi commented Nov 7, 2019

I think this might be worth doing, but it's only going to get less important as modules takes over and vendor directories become less common.

@glibsm
Copy link
Author

glibsm commented Nov 7, 2019

I'm going to close this issue, as ever since moving to modules this has been a non-issue.

@glibsm glibsm closed this as completed Nov 7, 2019
@golang golang locked and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants