cmd/go/internal/test: default pgo=auto results in surprising slow behaviour

[lizthegrey]

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

$ go version
go version go1.21.0 linux/amd64

Does this issue reproduce with the latest release?

yes.

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

go env Output

$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/lizf/.cache/go-build'
GOENV='/home/lizf/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/lizf/hny/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/lizf/hny/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/snap/go/current'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/snap/go/current/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build522135516=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Run go test ./... where one directory (which has no tests in it) contains a default.pgo file, but a subdirectory contains tests. eg:

cmd/bar/main.go
cmd/foo/default.pgo
cmd/foo/main.go
cmd/foo/internal/foo_test.go
shared_libs/some_libraries.go
...

What did you expect to see?

Build should be fast because all intermediate build artifacts are reused since no *_test.go files are directly located in a directory containing default.pgo

$ go test ./cmd/foo/...
(pause while shared libs are compiled)
?   	cmd/bar	[no test files]
(no pause)
?   	cmd/foo	[no test files]
(no pause)
ok  	cmd/foo/internal	0.013s

What did you see instead?

A custom build was done for cmd/app with default.pgo in use (even if no tests were found for that directory), forcing recompilation of everything to ensure complete optimization.

$ go test ./cmd/...
(pause while shared libs are compiled)
?   	cmd/bar	[no test files]
(long pause while shared libs are recompiled with pgo even if cmd/foo has no tests)
?   	cmd/foo	[no test files]
(no pause)
ok  	cmd/foo/internal	0.013s

However, running with pgo explicitly turned off does proceed quickly:

$ go test -pgo=off ./cmd/...
(pause while shared libs are compiled)
?   	cmd/bar	[no test files]
(no pause)
?   	cmd/foo	[no test files]
(no pause)
ok  	cmd/foo/internal	0.013s

[dmitshur]

CC @bcmills, @cherrymui.

[lizthegrey]

(this could be WAI if the intent here is that go test always compiles the directory using default settings even if no tests are present, but I found it frustrating to debug why my test step got infinitely slower)

[bcmills]

The documentation for the -pgo flag currently says:

When the special name "auto" is specified, for each main package in the
build, the go command selects a file named "default.pgo" in the package's
directory if that file exists, and applies it to the (transitive)
dependencies of the main package (other packages are not affected).

I agree that if no test program is actually being built for cmd/foo, then go test should not rebuild test variants of its imports either.

[bcmills]

@cherrymui, do you have the bandwidth to write a fix for this?

[mknyszek]

In triage, @thanm pointed out that it's probably vet forcing the rebuild. Can you reproduce with go test -vet=off?

[mknyszek]

@cherrymui suggests that maybe -pgo=auto should not turn on PGO by default for tests? That would resolve the issue, but it means PGO would not automatically be enabled for benchmarking.

@aclements suggests that the Go command could recognize that there are no test files and that it's only building the target for vet, and turn off PGO for that target.

[lizthegrey]

In triage, @thanm pointed out that it's probably vet forcing the rebuild. Can you reproduce with go test -vet=off?

No difference, extremely long "rebuild the world" pause with -vet=off as well.

$ go clean -cache
$ go test -vet=off ./cmd/...
[...]
?   	github.com/honeycombio/hound/cmd/retriever	[no test files]
FAIL	github.com/honeycombio/hound/cmd/retriever/app	0.053s
ok  	github.com/honeycombio/hound/cmd/retriever-cli/cli	0.053s
(pause)
?   	github.com/honeycombio/hound/cmd/retriever-lambda	[no test files]
?   	github.com/honeycombio/hound/cmd/retriever-lifecycle	[no test files]
?   	github.com/honeycombio/hound/cmd/retriever-scooper	[no test files]
?   	github.com/honeycombio/hound/cmd/retriever-terminator	[no test files]
ok  	github.com/honeycombio/hound/cmd/retriever-lambda/app	0.052s
ok  	github.com/honeycombio/hound/cmd/retriever-rollover	0.059s

[cherrymui]

Sorry for the delay. @lizthegrey could you try if CL https://go.dev/cl/531796 helps reduce the testing time? Thanks!

[gopherbot]

Change https://go.dev/cl/531796 mentions this issue: cmd/go: don't attach PGO profile for testing if there is no test

[lizthegrey]

Sorry for the delay. @lizthegrey could you try if CL go.dev/cl/531796 helps reduce the testing time? Thanks!

I'll have to build a go binary with gotip, it'll take me a few days to get to this.

[cherrymui]

@lizthegrey that CL should apply cleanly to Go 1.21 release branch, if that helps. Thanks.

[lizthegrey]

@lizthegrey that CL should apply cleanly to Go 1.21 release branch, if that helps. Thanks.

Verified working. It quickly shows [no test files] and moves right along.

[mvdan]

Friendly nudge, now that we are four weeks away from the 1.23 freeze :) Bryan gave a review on the patch back in November, and he's left the team at this point, but hopefully we should still be able to finish the review. cc @matloob @samthanawalla per https://dev.golang.org/owners.

[lizthegrey]

Friendly nudge, now that we are four weeks away from the 1.23 freeze :) Bryan gave a review on the patch back in November, and he's left the team at this point, but hopefully we should still be able to finish the review. cc @matloob @samthanawalla per dev.golang.org/owners.

Please confirm you don't need me to further test any patches, and that this is just a matter of review and merging.

[cherrymui]

Please confirm you don't need me to further test any patches, and that this is just a matter of review and merging.

Not at the moment. Thanks.

CL 531796 should fix the issue. But according to Bryan's comment this may not be the right approach. I'll look into it and see if there is a better approach. Thanks.