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

testing: benchmark output should include settings like GOAMD64 #53782

Open
mvdan opened this issue Jul 11, 2022 · 4 comments
Open

testing: benchmark output should include settings like GOAMD64 #53782

mvdan opened this issue Jul 11, 2022 · 4 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Jul 11, 2022

go test -bench has printed system information like goos, goarch, and cpu as part of benchmark results for some time now:

go/src/testing/benchmark.go

Lines 265 to 272 in 59ab6f3

fmt.Fprintf(b.w, "goos: %s\n", runtime.GOOS)
fmt.Fprintf(b.w, "goarch: %s\n", runtime.GOARCH)
if b.importPath != "" {
fmt.Fprintf(b.w, "pkg: %s\n", b.importPath)
}
if cpu := sysinfo.CPU.Name(); cpu != "" {
fmt.Fprintf(b.w, "cpu: %s\n", cpu)
}

As a companion to goarch and cpu, I think we should also include the value of Go environment variables like GOAMD64 - similar env vars include GOARM, GOMIPS64, etc. They are listed together in go help environment.

This matters just like cpu does: benchmark numbers will vary between CPUs, and some benchmarks might benefit significantly from GOAMD64=v3 compared to the default of GOAMD64=v1.

For the sake of reducing verbosity, we could only print these if they are set to a non-default value. GOAMD64=v1 wouldn't be printed, but GOAMD64=v3 would be.

Arguably there are lots of other factors that could contribute to higher or lower benchmark results, such as what kernel the sytem is running, which C toolchain was used to build cgo, what Go version is being used, etc. However, env vars like GOAMD64 feel much more in line with the goarch and cpu lines we already print, and they are a set of Go build options which directly affect performance.

@mknyszek mknyszek added FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 11, 2022
@mknyszek mknyszek added this to the Backlog milestone Jul 11, 2022
@mknyszek
Copy link
Contributor

CC @bcmills @aclements

I think this makes sense, and should be straightforward to do. I also don't think it would be too verbose to always print them (for the relevant platforms; i.e. don't print "GOARM" for "GOARCH=amd64"). We just need to pick names for the keys. Maybe just "goamd64," "gomips," etc.?

@mvdan
Copy link
Member Author

mvdan commented Jul 11, 2022

Yeah, just lowercasing the names seems good to me.

@martisch
Copy link
Contributor

I think we should print the default GOAMD64=v1 too as otherwise its not clear if it was the default or a Go version that never did include the GOAMD64 value in benchmark output.

@mvdan
Copy link
Member Author

mvdan commented Aug 12, 2022

Good point :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants