Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upcmd/go: `go env GOMOD` is too slow for use in interactive commands #29382
Comments
|
The algorithm has changed before, and will likely change again (whenever the default behavior switches over). Probably it's better to make I'm a total novice with the Linux From I get the following: |
|
I agree that'd be better, and I know Brad was looking at lazy-loading this kind of stuff, but in general keeping init functions out of something as big as the main go command seems pretty difficult. Maybe a new command in $GOROOT/bin, |
|
The lazy-loading issue was #26775. |
|
Let's see how far we can get making |
|
I actually was thinking the same when I saw I agree with the sentiment that milliseconds matter for some tools, but given that |
|
Change https://golang.org/cl/155540 mentions this issue: |
|
I had a stab at this and shaved a millisecond off on my machine (20%). The bigger offenders given by I wonder if packages like http and tls could reduce their init cost even further. If not, perhaps it does make sense to split I'm also a bit puzzled by how |
|
|
Interesting - but the Go benchmark is also executing the go binary at each iteration. Perhaps it's faster since it's loading the same binary over and over again. If anyone knows of a better way to write a benchmark for this, please let me know. Benchmarking a function directly, like the generated init function for the entire |
|
Change https://golang.org/cl/155961 mentions this issue: |
|
Change https://golang.org/cl/155962 mentions this issue: |
|
Change https://golang.org/cl/155963 mentions this issue: |
|
I've found a few more ways to remove work from I think we should be able to fairly easily shave off some more from I'll stop with the CLs for now, to get some input on the benchmark and style of the CLs. Happy to work on more once these have been approved. |
|
Retitling to reflect the current approach. |
bcmills
changed the title
|
Change https://golang.org/cl/164040 mentions this issue: |
This was implemented as part of go/doc, but it's going to be useful in other packages. In particular, many packages under cmd/go like web and vcs make somewhat heavy use of global regexes, which add a non-trivial amount of init work to the cmd/go program. A lazy wrapper around regexp.Regexp will make it trivial to get rid of the extra cost with a trivial refactor, so make it possible for other packages in the repository to make use of it. While naming the package, give the members better names, such as lazyregexp.New and lazyregexp.Regexp. We're also considering adding some form of a lazy API to the public regexp package, so this internal package will allow us to get some initial experience across std and cmd. For #29382. Change-Id: I30b0e72871d5267c309786f95f4cb15c68b2393d Reviewed-on: https://go-review.googlesource.com/c/164040 Run-TryBot: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
'go env' is used for many quick operations, such as in go/packages to query GOARCH and GOMOD. It often is a bottleneck; for example, go/packages doesn't know whether or not to use Go modules until it has queried GOMOD. As such, this go command should be fast. Right now it's slower than it should be. This commit adds a simple benchmark with os/exec, since we're particularly interested in the cost of cmd/go's large init function. Updates #29382. Change-Id: Ifee6fb9997b9b89565fbfc2739a00c86117b1d37 Reviewed-on: https://go-review.googlesource.com/c/155961 Run-TryBot: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
These regexes are all related to commands like get and build, so they're unnecessary for simpler commands like env. In particular, we need env to be fast, since libraries like go/packages call it early and often. Some external Go tools are interactive, so milliseconds matter. lazyregexp eagerly compiles the patterns when running from within a test binary, so there's no longer any need to do that as part of non-test binaries. Picking up the low-hanging fruit spotted by 'perf record' shaves off well over a full millisecond off the benchmark on my laptop: name old time/op new time/op delta ExecGoEnv-8 4.92ms ± 1% 3.81ms ± 0% -22.52% (p=0.004 n=6+5) This CL required adding a few more methods to the lazy regexp wrapper. Updates #29382. Change-Id: I22417ab6258f7437a2feea0d25ceb2bb4d735a15 Reviewed-on: https://go-review.googlesource.com/c/155540 Run-TryBot: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
Change https://golang.org/cl/164337 mentions this issue: |
Similar to internal/lazyregexp, this will allow removing unnecessary work from init functions with trivial refactors, thanks to sync.Once. Copy the structure. The only major difference is that a template also carries a name. For #29382. Change-Id: I65d096dc2e2072b310bf59a814cd62669856b5b5 Reviewed-on: https://go-review.googlesource.com/c/164337 Run-TryBot: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The template is over a hundred lines and full of pipelines, and text/template isn't optimised to parse quickly, so it's no wonder that delaying the parsing to the first template use makes 'go env' much faster. Like in the previous patches to get rid of global regexp.MustCompile vars, use the newly introduced lazytemplate package. Close to two full milliseconds are shaved off of 'go env' runs. name old time/op new time/op delta ExecGoEnv-8 4.27ms ± 0% 2.63ms ± 1% -38.43% (p=0.002 n=6+6) Updates #29382. Change-Id: I4e2569e51ddf2afe1b46eb1a9e9e5845f7a3b0bd Reviewed-on: https://go-review.googlesource.com/c/155962 Run-TryBot: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
All previous changes merged; A
I think the only big low-hanging fruit is What should the solution be? Not to collect stack frames if |
|
I filed #30468 for the general regression. In the meantime perhaps we should substitute an internal package for |
|
Thanks! I don't think this is terribly urgent; we won ~50% and lost ~20%, so I'd wait another week or two to see if #30468 can have a good solution. |
|
Change https://golang.org/cl/166459 mentions this issue: |
|
I've had a look around and managed to scrape another 3%. I don't think we're going to get much more without larger refactors, such as making If anyone is better at I'd say we should focus on #30468 before continuing; right now |
|
Change https://golang.org/cl/167401 mentions this issue: |
|
Change https://golang.org/cl/167400 mentions this issue: |
See Issue #29382 and Issue #30468. 3 frames are no longer needed as of https://go-review.googlesource.com/c/go/+/152537/ name old time/op new time/op delta New-8 475ns ± 3% 352ns ± 2% -25.87% (p=0.008 n=5+5) Errorf/no_format-8 661ns ± 4% 558ns ± 2% -15.63% (p=0.008 n=5+5) Errorf/with_format-8 729ns ± 6% 626ns ± 2% -14.23% (p=0.008 n=5+5) Errorf/method:_mytype-8 1.00µs ± 9% 0.84µs ± 2% -15.94% (p=0.008 n=5+5) Errorf/method:_number-8 1.25µs ± 7% 1.04µs ± 2% -16.38% (p=0.008 n=5+5) Change-Id: I30377e769b3b3be623f63ecbe365f8950ca08dda Reviewed-on: https://go-review.googlesource.com/c/go/+/167400 Run-TryBot: Marcel van Lohuizen <mpvl@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Damien Neil <dneil@google.com>
See Issue #29382 and Issue #30468. Improvements in this CL: name old time/op new time/op delta New-8 352ns ± 2% 225ns ± 5% -36.04% (p=0.008 n=5+5) Improvements together with moving to 1 uintptr: name old time/op new time/op delta New-8 475ns ± 3% 225ns ± 5% -52.59% (p=0.008 n=5+5) Change-Id: I9d69a14e5e10a6498767defb7d5f26ceedcf9ba5 Reviewed-on: https://go-review.googlesource.com/c/go/+/167401 Run-TryBot: Marcel van Lohuizen <mpvl@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Damien Neil <dneil@google.com>
|
I'm seeing 20ms for 'go env GOMOD'. Is that really too slow? |
|
It's gotten ~50% faster since the issue was opened :) At least as far as the benchmark tells us. |
|
Now that the CLs to make |
|
I'm seeing 11ms at this point. That's well below typical keypress latency for a USB keyboard,¹ and can generally be cached for a given working directory anyway. That seems good enough for interactive use. |
The first biggest offender was crypto/des.init at ~1%. It's cryptographically broken and the init function is relatively expensive, which is unfortunate as both crypto/tls and crypto/x509 (and by extension, cmd/go) import it. Hide the work behind sync.Once. The second biggest offender was flag.sortFlags at just under 1%, used by the Visit flagset methods. It allocated two slices, which made a difference as cmd/go iterates over multiple flagsets during init. Use a single slice with a direct sort.Interface implementation. Another big offender is initializing global maps. Reducing this work in cmd/go/internal/imports and net/textproto gives us close to another whole 1% in saved work. The former can use map literals, and the latter can hide the work behind sync.Once. Finally, compress/flate used newHuffmanBitWriter as part of init, which allocates many objects and slices. Yet it only used one of the slice fields. Allocating just that slice saves a surprising ~0.3%, since we generated a lot of unnecessary garbage. All in all, these little pieces amount to just over 3% saved CPU time. name old time/op new time/op delta ExecGoEnv-8 3.61ms ± 1% 3.50ms ± 0% -3.02% (p=0.000 n=10+10) Updates #26775. Updates #29382. Change-Id: I915416e88a874c63235ba512617c8aef35c0ca8b Reviewed-on: https://go-review.googlesource.com/c/go/+/166459 Run-TryBot: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
We're porting a lot of tools to support modules, and most of them need to know whether modules are enabled to turn on the newer, slower, potentially buggier code path. Running
go env GOMODtakes about 10ms, which is not negligible for an interactive command likegodeforguru. The algorithm for determining if modules are on is just simple enough to tempt people to roll their own. In fact, there's already a second implementation ingo/build:go/src/go/build/build.go
Line 977 in 429bae7
This is unfortunate, especially since the algorithm changed in http://golang.org/cl/148517. (Maybe not in a way that matters for go/build? Not sure.)
I think it'd be helpful to have a standalone internal package that implemented this functionality that could be used for
go env,go/build, and copied into x/tools for other tools to use.@bcmills @jayconrod @ianthehat