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

Significant performance issues with goimports and modules #263

Closed
rliebz opened this issue Oct 30, 2018 · 7 comments
Closed

Significant performance issues with goimports and modules #263

rliebz opened this issue Oct 30, 2018 · 7 comments

Comments

@rliebz
Copy link
Contributor

rliebz commented Oct 30, 2018

Running goimports outside of the GOPATH has a significant performance hit, off by an order of magnitude compared to other linters. No other linter seems to have any significant difference off the GOPATH.

I've run a quick benchmark against this repo after removing .golangci.yml for a clean configuration, with times taken from the -v output. I've gotten similar results after repeating multiple times, and similar results using another repo.

Some numbers, which seem to indicate that only goimports + modules has the issue:

command on GOPATH time (seconds)
golangci-lint run -v --enable-all --disable goimports ./... yes 5.671473167
golangci-lint run -v --disable-all --enable goimports ./... yes 3.489298274
golangci-lint run -v --enable-all --disable goimports ./... no 5.844223245
golangci-lint run -v --disable-all --enable goimports ./... no 46.860324028
  1. Version of golangci-lint: golangci-lint --version
$ golangci-lint --version
golangci-lint has version 1.11 built from 17508ab on 2018-10-28T15:21:50Z
  1. Go environment: go version && go env
$ go version && go env
go version go1.11.1 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/rliebz/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/rliebz/Projects/gocode"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.1/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/rliebz/Projects/tusk/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/wb/_ss422w92c78hrs6vf8456_r0000gn/T/go-build844491563=/tmp/go-build -gno-record-gcc-switches -fno-common"
  1. Verbose output of running: golangci-lint run -v --disable-all --enable goimports ./... with no .golangci.yml
INFO [config_reader] Config search paths: [./ /Users/rliebz/Playground/golangci-lint /Users/rliebz/Playground /Users/rliebz /Users /] 
INFO [lintersdb] Active 1 linters: [goimports]    
INFO [loader] Go packages loading at mode load files took 1.090516742s 
INFO [loader/astcache] Parsed AST of all pkg.GoFiles: [/Users/rliebz/Playground/golangci-lint/cmd/golangci-lint/main.go] for 151.122µs 
... (a lot of really fast AST parsing)
INFO [runner/skip dirs] sorted abs args: [/Users/rliebz/Playground/golangci-lint] 
INFO [runner] worker.6 took 12.8µs                
INFO [runner] worker.3 took 3.341µs               
INFO [runner] worker.7 took 4.922µs               
INFO [runner] worker.8 took 23.615µs              
INFO [runner] worker.5 took 26.35µs               
INFO [runner] worker.2 took 47.4µs                
INFO [runner] worker.1 took 56.66µs               
INFO [runner] worker.4 took 45.557783839s with stages: goimports: 45.557748359s 
INFO [runner] Workers idle times: #1: 45.557597123s, #2: 45.557601806s, #3: 45.557703824s, #5: 45.557607879s, #6: 45.557729237s, #7: 45.557666091s, #8: 45.557627599s 
INFO [runner] processing took 9.564µs with stages: nolint: 3.071µs, path_prettifier: 2.688µs, max_same_issues: 1.04µs, skip_dirs: 477ns, autogenerated_exclude: 359ns, skip_files: 264ns, cgo: 262ns, diff: 251ns, exclude: 247ns, max_from_linter: 239ns, max_per_file_from_linter: 186ns, source_code: 181ns, uniq_by_line: 165ns, path_shortener: 134ns 
INFO Memory: 469 samples, avg is 69.7MB, max is 70.1MB 
INFO Execution took 46.860324028s
@jirfag
Copy link
Member

jirfag commented Oct 30, 2018

hi, thank you for the verbose description!

  1. does it reproduce on the repeated run?
  2. how much lines of code does project have?
  3. how long simple find . -name "*.go" | xargs -t goimports -w work?
  4. is it much faster in GOPATH?

@jirfag
Copy link
Member

jirfag commented Oct 30, 2018

I have reproduced it myself

  1. yes
  2. 9 kLoC
  3. time sh -c 'find . -name "*.go"| fgrep -v vendor | xargs goimports -w' took 43s
  4. it's much faster in GOPATH

@jirfag
Copy link
Member

jirfag commented Oct 30, 2018

It's the issue with goimports.
They already have a WIP patch, I'll look at it and try to merge.

jirfag added a commit that referenced this issue Nov 5, 2018
Apply https://go-review.googlesource.com/c/tools/+/132598/ as a
temporary fix before a proper fix is in golang.org/x/tools
jirfag added a commit that referenced this issue Nov 5, 2018
Apply https://go-review.googlesource.com/c/tools/+/132598/ as a
temporary fix before a proper fix is in golang.org/x/tools
@jirfag jirfag closed this as completed in c02a6da Nov 5, 2018
@otto-md
Copy link

otto-md commented Nov 5, 2018

As part of this commit golang.org/x/tools was updated to 6c7e314b6563. This includes golang/tools@a2dc476 which reverted the very nice improvement to import grouping that was introduced in 0e2be20

Is there any way of getting this feature back? Having working import grouping is more important to me than keeping import comments, which was the reason for the revert upstream.

@jirfag
Copy link
Member

jirfag commented Nov 6, 2018

yep, it contains bugs (described in golang/go#20818). We could merge it when it would be fixed

@jirfag jirfag reopened this Nov 10, 2018
@jirfag
Copy link
Member

jirfag commented Nov 10, 2018

Reverting fix because it's buggy, waiting for a final fix in x/tools

jirfag added a commit that referenced this issue Dec 22, 2018
The new version of goimports works 100x faster
with go modules. Also it has some new features:

$ git cherry -v 6c7e314b6563 92cdcd90bf52 | fgrep imports
+ 5bbcdc15656ef390fab5dd6e8daf95354f7171e3 imports: redesign fixImports
+ ee45598d2ff288037f53f9e13ae0b1a6e2165ad5 imports: create named imports for name/path mismatches (again)
+ 4c53570e0460bc32468f75bf9dd71c018d03bfa9 imports: ignore globals in different packages
+ 1d424dbce8dd500e9e449fd3ff9d0668c09e2ae1 imports: clean up customization seam
+ 6a3e9aa2ab7749d72d1006ee484271b4a11f96c2 imports: fix renamed sibling imports
+ 5f4a60f04f23ac48e0665f257413ae3eacf339be imports: fix renamed sibling imports more
+ bbccd8cae4a9a47e0f978e03ff4b5df88a9fde1e imports: use go/packages, support modules
+ d4971274fe382404aee0e8c163af974f2bf738e6 imports: don't remove imports that conflict with globals
@jirfag jirfag closed this as completed in a4a7100 Dec 22, 2018
@jirfag
Copy link
Member

jirfag commented Dec 22, 2018

Merged a new fix for goimports, it should work fast again

jirfag added a commit that referenced this issue Feb 11, 2019
$ git cherry --abbrev -v 8afd9cbb6cfb 66fb7fc33547
+ 63b25c1 Fix typo in README (#235)
+ 419c929 G107 - SSRF (#236)
+ 145f1a0 Removed wrapping feature (#238)
+ ec32ce6 Support Go 1.11 (#239)
+ 762ff3a Allow quoted strings to be used to format SQL queries (#240)
+ 7f6509a Update README.md (#246)
+ 5f98926 Refactor Dockerfile (#245)
+ d3f1980 Fix false positives for SQL string concatenation with constants from another file (#247)
+ 64d58c2 Refactor the test code sample to support multiple files per sample
+ 1ecd47e bump Dockerfile golang from 1.10 to 1.11
+ 027dc2b This fixes the html template when using '-fmt=html'  - resolves HTML escaping issues within the template  - resolves reference issues to reportInfo struct i.e. issues -> Issues, metrics -> Stats
+ 8c09a83 Add install.sh script
+ 97bc137 Add CI Installation steps and correct markdown lint errors
+ 3116b07 Fix typos in comments and rulelist (#256)
+ 443f84f Fix golint link (#263)
+ 4180994 Make G201 ignore CallExpr with no args (#262)
+ 9b966a4 add test case for strings.Builder G104 whitelist inclusion
+ adb4222 whitelist strings.Builder method in rule G104
+ ae82798 Fix the WriteSring test by handling the error
+ 2695567 Build the code sample for string builder only fron Go 1.10 onwards
+ f14f17f Add a helper function which extracts the string parameters values of a call expression
+ 9b32fca Fix the bind rule to handle the case when the arguments of the net.Listen are returned by a function call
+ 24e3094 Extend the bind rule to handle the case when the net.Listen address in provided from a const
+ 72e95e8 Geneate and upload the test coverage report to codecove.io
+ 12400f9 Update README with the code coverage batch
+ 14ed63d Do not flag the unhandled errors which are explicitly ignored
+ f87af5f Detect the unhandled errors even though they are explicitly ignored if the 'audit: enabled' setting is defined in the global configuration (#274)
+ 5d33e6e Update the README with some details about the configuration file
+ b662615 Fix typo
+ a966ff7 Fix -conf example in README.md
+ 04ce7ba add a no-fail flag
+ e2752bc revert to default GOPATH if necessary (#279)
- c04360f make API
+ 66fb7fc Replace import paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants