benchfmt: add support for unit metadata This adds support for tracking unit metadata in Result, reading it in the Reader, and writing it back out in the Writer. Updates golang/go#43744. Change-Id: Id3311340b6e6992c149f9bca44c2e1e442b53eb4 Reviewed-on: https://go-review.googlesource.com/c/perf/+/357549 Trust: Austin Clements <austin@google.com> Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com>
diff --git a/benchfmt/files.go b/benchfmt/files.go index d23630a..973788d 100644 --- a/benchfmt/files.go +++ b/benchfmt/files.go
@@ -177,3 +177,9 @@ func (f *Files) Err() error { return f.err } + +// Units returns the accumulated unit metadata. +// See Reader.Units. +func (f *Files) Units() map[UnitMetadataKey]*UnitMetadata { + return f.reader.Units() +}
diff --git a/benchfmt/reader.go b/benchfmt/reader.go index 91464fb..32b0ec6 100644 --- a/benchfmt/reader.go +++ b/benchfmt/reader.go
@@ -29,8 +29,15 @@ s *bufio.Scanner err error // current I/O error - result Result - resultErr *SyntaxError + // q is the queue of records to return before processing the next + // input line. qPos is the index of the current record in q. We + // track the index explicitly rather than slicing q so that we can + // reuse the q slice when we reach the end. + q []Record + qPos int + + result Result + units UnitMetadataMap interns map[string]string } @@ -68,6 +75,7 @@ // Reset resets the reader to begin reading from a new input. // It also resets all accumulated configuration values. +// It does NOT reset unit metadata because it carries across files. // // initConfig is an alternating sequence of keys and values. // Reset will install these as the initial internal configuration @@ -78,10 +86,17 @@ fileName = "<unknown>" } r.err = nil - r.resultErr = noResult if r.interns == nil { r.interns = make(map[string]string) } + if r.units == nil { + r.units = make(map[UnitMetadataKey]*UnitMetadata) + } + + // Wipe the queue in case the user hasn't consumed everything from + // this file. + r.qPos = 0 + r.q = r.q[:0] // Wipe the Result. r.result.Config = r.result.Config[:0] @@ -103,7 +118,10 @@ } } -var benchmarkPrefix = []byte("Benchmark") +var ( + benchmarkPrefix = []byte("Benchmark") + unitPrefix = []byte("Unit") +) // Scan advances the reader to the next result and reports whether a // result was read. @@ -115,7 +133,19 @@ return false } - for r.s.Scan() { + // If there's anything in the queue from an earlier line, just pop + // the queue and return without consuming any more input. + if r.qPos+1 < len(r.q) { + r.qPos++ + return true + } + // Otherwise, we've drained the queue and need to parse more input + // to refill it. Reset it to 0 so we can reuse the space. + r.qPos = 0 + r.q = r.q[:0] + + // Process lines until we add something to the queue or hit EOF. + for len(r.q) == 0 && r.s.Scan() { r.result.line++ // We do everything in byte buffers to avoid allocation. line := r.s.Bytes() @@ -125,8 +155,20 @@ // At this point we commit to this being a // benchmark line. If it's malformed, we treat // that as an error. - r.resultErr = r.parseBenchmarkLine(line) - return true + if err := r.parseBenchmarkLine(line); err != nil { + r.q = append(r.q, err) + } else { + r.q = append(r.q, &r.result) + } + continue + } + if len(line) > 0 && line[0] == 'U' { + if nLine, ok := r.isUnitLine(line); ok { + // Parse unit metadata line. This queues up its own + // records and errors. + r.parseUnitLine(nLine) + continue + } } if key, val, ok := parseKeyValueLine(line); ok { // Intern key, since there tend to be few @@ -143,6 +185,12 @@ // Ignore the line. } + if len(r.q) > 0 { + // We queued something up to return. + return true + } + + // We hit EOF. Check for IO errors. if err := r.s.Err(); err != nil { r.err = fmt.Errorf("%s:%d: %w", r.result.fileName, r.result.line, err) return false @@ -257,6 +305,68 @@ return nil } +// isUnitLine tests whether line is a unit metadata line. If it is, it +// returns the line after the "Unit" literal and true. +func (r *Reader) isUnitLine(line []byte) (rest []byte, ok bool) { + var f []byte + // Is this a unit metadata line? + f, line = splitField(line) + if bytes.Equal(f, unitPrefix) { + return line, true + } + return nil, false +} + +// parseUnitLine parses line as a unit metadata line, starting +// after "Unit". It updates r.q. +// If there are syntax errors on the line, it will attempt to parse +// what it can and return a non-nil error. +func (r *Reader) parseUnitLine(line []byte) { + var f []byte + // isUnitLine already consumed the literal "Unit". + // Consume the next field, which is the unit. + f, line = splitField(line) + if len(f) == 0 { + r.q = append(r.q, r.newSyntaxError("missing unit")) + return + } + unit := r.intern(f) + + // The metadata map is indexed by tidied units because we want to + // support lookups by tidy units and there's no way to "untidy" a + // unit. + _, tidyUnit := benchunit.Tidy(1, unit) + + // Consume key=value pairs. + for { + f, line = splitField(line) + if len(f) == 0 { + break + } + eq := bytes.IndexByte(f, '=') + if eq <= 0 { + r.q = append(r.q, r.newSyntaxError("expected key=value")) + continue + } + key := UnitMetadataKey{tidyUnit, r.intern(f[:eq])} + value := r.intern(f[eq+1:]) + + if have, ok := r.units[key]; ok { + if have.Value == value { + // We already have this unit metadata. Ignore. + continue + } + // Report incompatible unit metadata. + r.q = append(r.q, r.newSyntaxError(fmt.Sprintf("metadata %s of unit %s already set to %s", key.Key, unit, have.Value))) + continue + } + + metadata := &UnitMetadata{key, unit, value, r.result.fileName, r.result.line} + r.units[key] = metadata + r.q = append(r.q, metadata) + } +} + func (r *Reader) intern(x []byte) string { const maxIntern = 1024 if s, ok := r.interns[string(x)]; ok { @@ -289,9 +399,10 @@ var _ Record = (*Result)(nil) var _ Record = (*SyntaxError)(nil) +var _ Record = (*UnitMetadata)(nil) // Result returns the record that was just read by Scan. This is either -// a *Result or a *SyntaxError indicating a parse error. +// a *Result, a *UnitMetadata, or a *SyntaxError indicating a parse error. // It may return more types in the future. // // Parse errors are non-fatal, so the caller can continue to call @@ -300,10 +411,11 @@ // If this returns a *Result, the caller should not retain the Result, // as it will be overwritten by the next call to Scan. func (r *Reader) Result() Record { - if r.resultErr != nil { - return r.resultErr + if r.qPos >= len(r.q) { + // This should only happen if Scan has never been called. + return noResult } - return &r.result + return r.q[r.qPos] } // Err returns the first non-EOF I/O error that was encountered by the @@ -312,6 +424,15 @@ return r.err } +// Units returns the accumulated unit metadata. +// +// Callers that want to consume the entire stream of benchmark results +// and then process units can use this instead of monitoring +// *UnitMetadata Records. +func (r *Reader) Units() UnitMetadataMap { + return r.units +} + // Parsing helpers. // // These are designed to leverage common fast paths. The ASCII fast
diff --git a/benchfmt/reader_test.go b/benchfmt/reader_test.go index 8ea6243..9636dff 100644 --- a/benchfmt/reader_test.go +++ b/benchfmt/reader_test.go
@@ -17,7 +17,7 @@ "time" ) -func parseAll(t *testing.T, data string, setup ...func(r *Reader, sr io.Reader)) []Record { +func parseAll(t *testing.T, data string, setup ...func(r *Reader, sr io.Reader)) ([]Record, *Reader) { sr := strings.NewReader(data) r := NewReader(sr, "test") for _, f := range setup { @@ -32,7 +32,7 @@ res.fileName = "" res.line = 0 out = append(out, res) - case *SyntaxError: + case *SyntaxError, *UnitMetadata: out = append(out, rec) default: t.Fatalf("unexpected result type %T", rec) @@ -41,7 +41,7 @@ if err := r.Err(); err != nil { t.Fatal("parsing failed: ", err) } - return out + return out, r } func printRecord(w io.Writer, r Record) { @@ -55,6 +55,8 @@ fmt.Fprintf(w, " %v %s", val.Value, val.Unit) } fmt.Fprintf(w, "\n") + case *UnitMetadata: + fmt.Fprintf(w, "Unit: %+v\n", r) case *SyntaxError: fmt.Fprintf(w, "SyntaxError: %s\n", r) default: @@ -186,6 +188,10 @@ BenchmarkMissingUnit 100 1 BenchmarkMissingUnit2 100 1 ns/op 2 also not a benchmark +Unit +Unit ns/op blah +Unit ns/op a=1 +Unit ns/op a=2 `, []Record{ &SyntaxError{"test", 2, "missing iteration count"}, @@ -195,6 +201,10 @@ &SyntaxError{"test", 6, "parsing measurement: invalid syntax"}, &SyntaxError{"test", 7, "missing units"}, &SyntaxError{"test", 8, "missing units"}, + &SyntaxError{"test", 10, "missing unit"}, + &SyntaxError{"test", 11, "expected key=value"}, + &UnitMetadata{UnitMetadataKey{"sec/op", "a"}, "ns/op", "1", "test", 12}, + &SyntaxError{"test", 13, "metadata a of unit ns/op already set to 1"}, }, }, { @@ -221,16 +231,36 @@ v(1, "ns/op").res, }, }, + { + "unit metadata", + `Unit ns/op a=1 b=2 +Unit ns/op c=3 error d=4 +# Repeated unit should report nothing +Unit ns/op d=4 +# Starts like a unit line but actually isn't +Unitx +BenchmarkOne 100 1 ns/op +`, + []Record{ + &UnitMetadata{UnitMetadataKey{"sec/op", "a"}, "ns/op", "1", "test", 1}, + &UnitMetadata{UnitMetadataKey{"sec/op", "b"}, "ns/op", "2", "test", 1}, + &UnitMetadata{UnitMetadataKey{"sec/op", "c"}, "ns/op", "3", "test", 2}, + &SyntaxError{"test", 2, "expected key=value"}, + &UnitMetadata{UnitMetadataKey{"sec/op", "d"}, "ns/op", "4", "test", 2}, + r("One", 100). + v(1, "ns/op").res, + }, + }, } { t.Run(test.name, func(t *testing.T) { - got := parseAll(t, test.input) + got, _ := parseAll(t, test.input) compareRecords(t, got, test.want) }) } } func TestReaderInternalConfig(t *testing.T) { - got := parseAll(t, ` + got, _ := parseAll(t, ` # Test initial internal config Benchmark1 100 1 ns/op # Overwrite internal config with file config
diff --git a/benchfmt/units.go b/benchfmt/units.go new file mode 100644 index 0000000..1bc1f1e --- /dev/null +++ b/benchfmt/units.go
@@ -0,0 +1,99 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package benchfmt + +import ( + "golang.org/x/perf/benchmath" + "golang.org/x/perf/benchunit" +) + +// Unit metadata is a single piece of unit metadata. +// +// Unit metadata gives information that's useful to interpreting +// values in a given unit. The following metadata keys are predefined: +// +// better={higher,lower} indicates whether higher or lower values of +// this unit are better (indicate an improvement). +// +// assume={nothing,exact} indicates what statistical assumption to +// make when considering distributions of values. +// `nothing` means to make no statistical assumptions (e.g., use +// non-parametric methods) and `exact` means to assume measurements are +// exact (repeated measurement does not increase confidence). +// The default is `nothing`. +type UnitMetadata struct { + UnitMetadataKey + + // OrigUnit is the original, untidied unit as written in the input. + OrigUnit string + + Value string + + fileName string + line int +} + +func (u *UnitMetadata) Pos() (fileName string, line int) { + return u.fileName, u.line +} + +// UnitMetadataKey identifies a single piece of unit metadata by unit +// and metadata name. +type UnitMetadataKey struct { + Unit string + Key string // Metadata key (e.g., "better" or "assume") +} + +// UnitMetadataMap stores the accumulated metadata for several units. +// This is indexed by tidied units, while the values store the original +// units from the benchmark file. +type UnitMetadataMap map[UnitMetadataKey]*UnitMetadata + +// Get returns the unit metadata for the specified unit and metadata key. +// It tidies unit if necessary. +func (m UnitMetadataMap) Get(unit, key string) *UnitMetadata { + _, tidyUnit := benchunit.Tidy(1, unit) + return m[UnitMetadataKey{tidyUnit, key}] +} + +// GetAssumption returns the appropriate statistical Assumption to make +// about distributions of values in the given unit. +func (m UnitMetadataMap) GetAssumption(unit string) benchmath.Assumption { + dist := m.Get(unit, "assume") + if dist != nil && dist.Value == "exact" { + return benchmath.AssumeExact + } + // The default is to assume nothing. + return benchmath.AssumeNothing +} + +// GetBetter returns whether higher or lower values of the given unit +// indicate an improvement. It returns +1 if higher values are better, +// -1 if lower values are better, or 0 if unknown. +func (m UnitMetadataMap) GetBetter(unit string) int { + better := m.Get(unit, "better") + if better != nil { + switch better.Value { + case "higher": + return 1 + case "lower": + return -1 + } + return 0 + } + // Fall back to some built-in defaults. + switch unit { + case "ns/op", "sec/op": + // This measures "duration", so lower is better. + return -1 + case "MB/s", "B/s": + // This measures "speed", so higher is better. + return 1 + case "B/op", "allocs/op": + // These measure amount of allocation, so lower is better. + return -1 + } + return 0 +}
diff --git a/benchfmt/units_test.go b/benchfmt/units_test.go new file mode 100644 index 0000000..fa56963 --- /dev/null +++ b/benchfmt/units_test.go
@@ -0,0 +1,99 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package benchfmt + +import ( + "testing" + + "golang.org/x/perf/benchmath" +) + +func TestReaderUnits(t *testing.T) { + _, reader := parseAll(t, ` +Unit ns/op a=1 b=2 +Unit MB/s c=3 c=3 +`) + units := reader.Units() + if len(units) != 3 { + t.Fatalf("expected 3 unit metadata, got %d:\n%v", len(units), units) + } + check := func(unit, tidyUnit, key, value string) { + t.Helper() + // The map is indexed by tidy units. + mkey := UnitMetadataKey{tidyUnit, key} + m, ok := units[mkey] + if !ok { + t.Errorf("for %s:%s, want %s, got none", tidyUnit, key, value) + return + } + want := UnitMetadata{UnitMetadataKey: mkey, OrigUnit: unit, Value: value} + if m.UnitMetadataKey != want.UnitMetadataKey || + m.OrigUnit != want.OrigUnit || + m.Value != want.Value { + t.Errorf("for %s:%s, want %+v, got %+v", unit, key, want, m) + } + // Check that Get works for both tidied and untidied units. + if got := units.Get(unit, key); got != m { + t.Errorf("for Get(%v, %v), want %+v, got %+v", unit, key, m, got) + } + if got := units.Get(tidyUnit, key); got != m { + t.Errorf("for Get(%v, %v), want %+v, got %+v", tidyUnit, key, m, got) + } + } + check("ns/op", "sec/op", "a", "1") + check("ns/op", "sec/op", "b", "2") + check("MB/s", "B/s", "c", "3") +} + +func TestUnitsGetAssumption(t *testing.T) { + _, reader := parseAll(t, ` +Unit a assume=nothing +Unit ns/frob assume=exact +Unit c assume=blah`) + units := reader.Units() + + check := func(unit string, want benchmath.Assumption) { + got := units.GetAssumption(unit) + if got != want { + t.Errorf("for unit %s, want assumption %s, got %s", unit, want, got) + } + } + check("a", benchmath.AssumeNothing) + check("ns/frob", benchmath.AssumeExact) + check("sec/frob", benchmath.AssumeExact) + check("c", benchmath.AssumeNothing) +} + +func TestUnitsGetBetter(t *testing.T) { + _, reader := parseAll(t, ` +Unit a better=higher +Unit b better=lower`) + units := reader.Units() + + check := func(unit string, want int) { + got := units.GetBetter(unit) + if got != want { + t.Errorf("for unit %s, want better=%+d, got %+d", unit, want, got) + } + } + check("a", 1) + check("b", -1) + check("c", 0) + + check("ns/op", -1) + check("sec/op", -1) + + check("MB/s", 1) + check("B/s", 1) + + check("B/op", -1) + check("allocs/op", -1) + + // Check that we can override the built-ins + _, reader = parseAll(t, "Unit ns/op better=higher") + units = reader.Units() + check("ns/op", 1) + check("sec/op", 1) +}
diff --git a/benchfmt/writer.go b/benchfmt/writer.go index 24b38b7..2c5ccc4 100644 --- a/benchfmt/writer.go +++ b/benchfmt/writer.go
@@ -33,15 +33,24 @@ func (w *Writer) Write(rec Record) error { switch rec := rec.(type) { case *Result: - return w.writeResult(rec) + w.writeResult(rec) + case *UnitMetadata: + w.writeUnitMetadata(rec) case *SyntaxError: // Ignore return nil + default: + return fmt.Errorf("unknown Record type %T", rec) } - return fmt.Errorf("unknown Record type %T", rec) + + // Flush the buffer out to the io.Writer. Write to the buffer + // can't fail, so we only have to check if this fails. + _, err := w.w.Write(w.buf.Bytes()) + w.buf.Reset() + return err } -func (w *Writer) writeResult(res *Result) error { +func (w *Writer) writeResult(res *Result) { // If any file config changed, write out the changes. if len(w.fileConfig) != len(res.Config) { w.writeFileConfig(res) @@ -66,12 +75,6 @@ w.buf.WriteByte('\n') w.first = false - - // Flush the buffer out to the io.Writer. Write to the buffer - // can't fail, so we only have to check if this fails. - _, err := w.w.Write(w.buf.Bytes()) - w.buf.Reset() - return err } func (w *Writer) writeFileConfig(res *Result) { @@ -127,3 +130,7 @@ w.buf.WriteByte('\n') } + +func (w *Writer) writeUnitMetadata(m *UnitMetadata) { + fmt.Fprintf(&w.buf, "Unit %s %s=%s\n", m.OrigUnit, m.Key, m.Value) +}
diff --git a/benchfmt/writer_test.go b/benchfmt/writer_test.go index 1798e5c..de0ffc0 100644 --- a/benchfmt/writer_test.go +++ b/benchfmt/writer_test.go
@@ -12,6 +12,9 @@ func TestWriter(t *testing.T) { const input = `BenchmarkOne 1 1 ns/op +Unit ns/op a=1 +Unit ns/op b=2 +Unit MB/s c=3 key: val key1: val1