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.

Sign up for GitHub

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

Jump to bottom

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

Open
lizthegrey opened this issue Aug 12, 2023 · 13 comments
Open

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

lizthegrey opened this issue Aug 12, 2023 · 13 comments
Assignees
@cherrymui
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. ToolSpeed
Milestone
Backlog

Comments

@lizthegrey
Copy link

lizthegrey commented Aug 12, 2023 •
edited

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

CC @bcmills, @cherrymui.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Aug 14, 2023
@dmitshur dmitshur added this to the Backlog milestone Aug 14, 2023
@lizthegrey
Copy link
Author

lizthegrey commented Aug 14, 2023 •
edited

(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 bcmills added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 14, 2023
@bcmills
Copy link
Contributor

bcmills commented Aug 14, 2023

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 bcmills added ToolSpeed NeedsFix The path to resolution is known, but the work has not been done. and removed Performance labels Aug 14, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 14, 2023
@bcmills
Copy link
Contributor

bcmills commented Aug 14, 2023

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

@mknyszek
Copy link
Contributor

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

@mknyszek
Copy link
Contributor

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

lizthegrey commented Aug 16, 2023 •
edited

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

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

@gopherbot
Copy link

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

@lizthegrey
Copy link
Author

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

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

@lizthegrey
Copy link
Author

lizthegrey commented Oct 12, 2023 •
edited

@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
Copy link
Member

mvdan commented Apr 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cherrymui cherrymui

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. ToolSpeed
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
7 participants
@lizthegrey @mknyszek @dmitshur @mvdan @bcmills @gopherbot @cherrymui