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: too slow #16367

Closed
romeovs opened this issue Jul 14, 2016 · 7 comments
Closed

x/tools/cmd/goimports: too slow #16367

romeovs opened this issue Jul 14, 2016 · 7 comments
Assignees

Comments

@romeovs
Copy link

@romeovs romeovs commented Jul 14, 2016

Please answer these questions before submitting your issue. Thanks!

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

    go version go1.6.2 darwin/amd64
    
  2. What operating system and processor architecture are you using (go env)?

    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="darwin"
    GOOS="darwin"
    GOPATH="/Users/romeo/code/go"
    GORACE=""
    GOROOT="/usr/local/Cellar/go/1.6.2/libexec"
    GOTOOLDIR="/usr/local/Cellar/go/1.6.2/libexec/pkg/tool/darwin_amd64"
    GO15VENDOREXPERIMENT="1"
    CC="clang"
    GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
    CXX="clang++"
    CGO_ENABLED="1"
    
  3. What did you do?
    I've got a repo with a lot of vendored libraries:

    $ tree vendor -d -L 2                                                                                     
    vendor
    ├── github.com
    │   ├── BurntSushi
    │   ├── TheThingsNetwork
    │   ├── apex
    │   ├── boltdb
    │   ├── brocaar
    │   ├── dgrijalva
    │   ├── eclipse
    │   ├── fsnotify
    │   ├── golang
    │   ├── googollee
    │   ├── gorilla
    │   ├── hashicorp
    │   ├── jacobsa
    │   ├── labstack
    │   ├── magiconair
    │   ├── mattn
    │   ├── mitchellh
    │   ├── rcrowley
    │   ├── robertkrimen
    │   ├── smartystreets
    │   ├── spf13
    │   ├── tj
    │   └── valyala
    ├── golang.org
    │   └── x
    ├── google.golang.org
    │   ├── appengine
    │   ├── cloud
    │   └── grpc
    └── gopkg.in
        ├── bsm
        ├── redis.v3
        ├── sourcemap.v1
        └── yaml.v2
    
    35 directories
    

    And now goimports is really slow:

    $ time goimports routes/events.go 1>/dev/null
    goimports routes/events.go > /dev/null  3.45s user 9.69s system 331% cpu 3.964 total
    

    However if I remove the vendored library and get the dependencies into my $GOPATH:

    rm -r vendor
    go get
    

    goimports is really fast again:

    $ time goimports routes/events.go 1>/dev/null
    goimports routes/events.go  1>/dev/null  0.01s user 0.01s system 68% cpu 0.032 total
    
  4. What did you expect to see?
    Same performance on vendored libs and libs in $GOPATH.

  5. What did you see instead?
    A large performance difference.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 14, 2016

Good timing. I was annoyed by goimports' performance recently and started investigating today during the Gophercon hack day. I have a change on my laptop to make things much faster.

It's doing way more work than necessary.

@bradfitz bradfitz self-assigned this Jul 14, 2016
@bradfitz bradfitz changed the title x/tools/cmd/goimports slow when using a lot of vendored libs x/tools/cmd/goimports: too slow Jul 14, 2016
@josharian
Copy link
Contributor

@josharian josharian commented Jul 14, 2016

Can't wait for the CL.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 15, 2016

CL https://golang.org/cl/24941 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Jul 15, 2016
I felt the burn of my laptop on my legs, spinning away while processing
goimports, and felt that it was time to make goimports great again.

Over the past few years goimports fell into a slow state of disrepair
with too many feature additions and no attention to the performance
death by a thousand cuts. This was particularly terrible on OS X with
its lackluster filesystem buffering.

This CL makes goimports stronger, together with various optimizations
and more visibility into what goimports is doing.

* adds more internal documentation

* avoids scanning $GOPATH for answers when running goimports on a file
  under $GOROOT (for Go core hackers)

* don't read all $GOROOT & $GOPATH directories' Go code looking for
  their package names until much later. Require the package name of
  missing imports to be present in the last two directory path
  components.  Then only try importing them in order from best to
  worst (shortest to longest, as before), so we can stop early.

* when adding imports, add names to imports when the imported package name
  doesn't match the baes of its import path. For example:
        import foo "example.net/foo/v1"

* don't read all *.go files in a package directory once the first file
  in a directory has revealed itself to be a package we're not looking
  for. For example, if we're looking for the right "client" for "client.Foo",
  we used to consider a directory "bar/client" as a candidate and read
  all 50 of its *.go files instead of stopping after its first *.go
  file had a "package main" line.

* add some fast paths to remove allocations

* add some fast paths to remove disk I/O when looking up the base
  package name of a standard library import (of existing imports in a
  file, which are very common)

* adds a special case for import "C", to avoid some disk I/O.

* add a -verbose flag to goimports for debugging

On my Mac laptop with a huge $GOPATH, with a test file like:

	package foo
	import (
	       "fmt"
	       "net/http"
	)

	/*

	*/
	import "C"

	var _ = cloudbilling.New
	var _ = http.NewRequest
	var _ = client.New

... this took like 10 seconds before, and now 1.3 seconds. (Still
slow; disk-based caching can come later)

Updates golang/go#16367 (goimports is slow)
Updates golang/go#16384 (refactor TestRename is broken on Windows)

Change-Id: I97e85d3016afc9f2ad5501f97babad30c7989183
Reviewed-on: https://go-review.googlesource.com/24941
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 15, 2016

CL https://golang.org/cl/24948 mentions this issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 15, 2016

CL https://golang.org/cl/24971 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Jul 15, 2016
When goimports was run on a file like:

    package main

    import (
        "example.net/foo"
        "example.net/bar"
    )

    var _, _ = foo.Foo, bar.Bar

... even though there looks to be no work to do, it still needs to
verify that "example.net/foo" is really package "foo" (even though it
looks like it) and "example.net/bar" is really package
"bar". (Packages in the standard library are hard-coded since the
previous commit and not verified for consistency since they're always consistent)

To do that verification for non-std packages, go/build.Import was
being used before, but Import reads all files in the directory to make
sure they're consistent. That's unnecessary. Instead, stop after the
first file. If example.net/foo has foo.go with "package foo" and
just_kidding.go with "package other", we never read that far to find
the inconsistency. Oh well. Prefer speed.

Updates golang/go#16367

Change-Id: I9fc3fefbee0e8a6bc287bf2a565257fb9523fd5c
Reviewed-on: https://go-review.googlesource.com/24948
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jul 15, 2016
Each $GOPATH entry may have a file $GOPATH/src/.goimportsignore which
may contain blank lines, #comment lines, or lines naming a directory
relative to the configuration file to ignore when scanning.  No
globbing or regex patterns are allowed.

Updates golang/go#16367 (goimports speed)
Fixes golang/go#16386 (add mechanism to ignore directories)

Change-Id: I8f1a88ae6c4d0ed3075444d70aec3e2228c5ce6a
Reviewed-on: https://go-review.googlesource.com/24971
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 18, 2016

CL https://golang.org/cl/25001 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Jul 19, 2016
This brings goimports from 160ms to 100ms on my laptop, and under 50ms
on my Linux machine.

Using cmd/trace, I noticed that filepath.Walk is inherently slow.
See https://golang.org/issue/16399 for details.

Instead, this CL introduces a new (private) filepath.Walk
implementation, optimized for speed and avoiding unnecessary work.

In addition to avoid an Lstat per file, it also reads directories
concurrently. The old goimports code did that too, but now that logic
is removed from goimports and the code is simplified.

This also adds some profiling command line flags to goimports that I
found useful.

Updates golang/go#16367 (goimports is slow)
Updates golang/go#16399 (filepath.Walk is slow)

Change-Id: I708d570cbaad3fa9ad75a12054f5a932ee159b84
Reviewed-on: https://go-review.googlesource.com/25001
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 21, 2016

I think it's fast enough to close this now.

@bradfitz bradfitz closed this Jul 21, 2016
@golang golang locked and limited conversation to collaborators Jul 21, 2017
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
4 participants
You can’t perform that action at this time.