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

x/perf/cmd/benchstat: units inconsistent with column header when time per op exceeds one second #61262

Open
bcmills opened this issue Jul 10, 2023 · 5 comments
Open

x/perf/cmd/benchstat: units inconsistent with column header when time per op exceeds one second #61262

bcmills opened this issue Jul 10, 2023 · 5 comments
Assignees
@aclements
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Jul 10, 2023

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

~/x/tools$ go version -m $(which benchstat)
/usr/local/google/home/bcmills/bin/benchstat: devel go1.21-b3ca8d2b3c Mon Jun 26 17:08:05 2023 +0000
        path    golang.org/x/perf/cmd/benchstat
        mod     golang.org/x/perf       v0.0.0-20230427221525-d343f6398b76      h1:cPGZx8Liyx5Pq/yX80/6WMKe2yidT0xvVCQBOGa8WHU=
        dep     github.com/aclements/go-moremath        v0.0.0-20210112150236-f10218a38794      h1:xlwdaKcTNVW4PtpQb8aKA4Pjy0CdJHEqvFbAnvR5m2g=
        build   -buildmode=exe
        build   -compiler=gc
        build   DefaultGODEBUG=panicnil=1
        build   CGO_ENABLED=1
        build   CGO_CFLAGS=
        build   CGO_CPPFLAGS=
        build   CGO_CXXFLAGS=
        build   CGO_LDFLAGS=
        build   GOARCH=amd64
        build   GOOS=linux
        build   GOAMD64=v1

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='/usr/local/google/home/bcmills/.cache/go-build'
GOENV='/usr/local/google/home/bcmills/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/usr/local/google/home/bcmills/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/usr/local/google/home/bcmills'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/google/home/bcmills/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/google/home/bcmills/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.21-b3ca8d2b3c Mon Jun 26 17:08:05 2023 +0000'
GCCGO='/usr/bin/gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/usr/local/google/home/bcmills/x/tools/go.mod'
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-build1213855422=/tmp/go-build -gno-record-gcc-switches'

What did you do?

~/x/tools$ go test -bench=. -run='^$' -count=10 ./internal/imports > ./cl508505.txt
~/x/tools$ git rebase --continue
~/x/tools$ go test -bench=. -run='^$' -count=10 ./internal/imports > ./cl508506.txt
~/x/tools$ benchstat cl508505.txt cl508506.txt

What did you expect to see?

Column headers consistent with the data within each column.

What did you see instead?

~/x/tools$ benchstat cl508505.txt cl508506.txt
goos: linux
goarch: amd64
pkg: golang.org/x/tools/internal/imports
cpu: AMD EPYC 7B12
                │ cl508505.txt │            cl508506.txt             │
                │    sec/op    │   sec/op     vs base                │
ScanModCache-24    231.0m ± 1%   264.4m ± 1%  +14.47% (p=0.000 n=10)

Note that the column headers report sec/op, but then the column data includes an m suffix, suggesting either minutes or milliseconds.

If the unit is going to be repeated in the individual entries for the column, it should be omitted from the column header.

On the other hand, if all of the entries are normalized to the same unit, the entries should not also include a unit suffix — especially not one for a unit of a different size!
@gopherbot gopherbot added this to the Unreleased milestone Jul 10, 2023
@bcmills bcmills added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 10, 2023
@rsc
Copy link
Contributor

rsc commented Jul 10, 2023

Can you show the input text files?
Why do you think the time/op exceeds one second?
Surely the time per op is not 264 minutes. It must be 264ms?

As a casual (not regular) user of benchstat this confuses me each time I see it too.
I understand the generic separation between unit and scale, but even so,
I think it would be much clearer to see time/op and 264ms than decode "sec/op and 264m".

This doesn't happen with bytes/op because 50k seems about as clear as 50kB.
Is time the only unit that gets subdivided? Perhaps it is worth a special case.

@rsc
Copy link
Contributor

rsc commented Jul 10, 2023

(In my typical benchmarks I am by now used to seeing "5.2n" which I inferred meant nanoseconds the first time I saw it. But "n" does not have the ambiguity that "m" does.)

@bcmills
Copy link
Contributor Author

bcmills commented Jul 10, 2023

Input files attached:
cl508505.txt
cl508506.txt

@bcmills
Copy link
Contributor Author

bcmills commented Jul 10, 2023

It is indeed milliseconds rather than minutes. So that's a bit less surprising, at least. 😅

@aclements
Copy link
Member

That's an interesting ambiguity with "minutes". Benchstat would never show something in "minutes", but I can see how it would be confusing.

On the other hand, if all of the entries are normalized to the same unit, the entries should not also include a unit suffix — especially not one for a unit of a different size!

I'm not sure I understand this. The "sec/op" column will always say "sec/op", regardless of the scale of the values. The cells do use a common scale across a row, but not within a column, so there's not a clear way to move the scale suffix into the column header. (Usually the rows are different benchmarks, which might have very different scales.)

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

@aclements aclements

Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@rsc @aclements @bcmills @gopherbot