diff --git a/test/benchmarks/tools/parser_util.go b/test/benchmarks/tools/parser_util.go index dac7aa5a4..22bffd315 100644 --- a/test/benchmarks/tools/parser_util.go +++ b/test/benchmarks/tools/parser_util.go @@ -48,26 +48,80 @@ func ParametersToName(params ...Parameter) (string, error) { } // NameToParameters parses the string created by ParametersToName and returns -// it as a set of Parameters. -// Example: BenchmarkRuby/server_threads.1/doc_size.16KB-6 -// The parameter part of this benchmark is: -// "server_threads.1/doc_size.16KB" (BenchmarkRuby is the name, and 6 is GOMAXPROCS) -// This function will return a slice with two parameters -> -// {Name: server_threads, Value: 1}, {Name: doc_size, Value: 16KB} -func NameToParameters(name string) ([]*Parameter, error) { +// the name components and parameters contained within. +// The separator between the name and value may either be '.' or '='. +// +// Example: "BenchmarkRuby/SubTest/LevelTwo/server_threads.1/doc_size.16KB-6" +// The parameter part of this benchmark is "server_threads.1/doc_size.16KB", +// whereas "BenchmarkRuby/SubTest/LevelTwo" is the name, and the "-6" suffix is +// GOMAXPROCS (optional, may be omitted). +// This function will return a slice of the name components of the benchmark: +// +// [ +// "BenchmarkRuby", +// "SubTest", +// "LevelTwo", +// ] +// +// and a slice of the parameters: +// +// [ +// {Name: "server_threads", Value: "1"}, +// {Name: "doc_size", Value: "16KB"}, +// {Name: "GOMAXPROCS", Value: "6"}, +// ] +// +// (and a nil error). +func NameToParameters(name string) ([]string, []*Parameter, error) { var params []*Parameter - for _, cond := range strings.Split(name, "/") { - cs := strings.Split(cond, ".") + var separator string + switch { + case strings.IndexRune(name, '.') != -1 && strings.IndexRune(name, '=') != -1: + return nil, nil, fmt.Errorf("ambiguity while parsing parameters from benchmark name %q: multiple types of parameter separators are present", name) + case strings.IndexRune(name, '.') != -1: + separator = "." + case strings.IndexRune(name, '=') != -1: + separator = "=" + default: + // No separator; use '=' which we know is not present in the name, + // but we still need to process the name (even if unparameterized) in + // order to possibly extract GOMAXPROCS. + separator = "=" + } + var nameComponents []string + var firstParameterCond string + var goMaxProcs *Parameter + split := strings.Split(name, "/") + for i, cond := range split { + if isLast := i == len(split)-1; isLast { + // On the last component, if it contains a dash, it is a GOMAXPROCS value. + if dashSplit := strings.Split(cond, "-"); len(dashSplit) >= 2 { + goMaxProcs = &Parameter{Name: "GOMAXPROCS", Value: dashSplit[len(dashSplit)-1]} + cond = strings.Join(dashSplit[:len(dashSplit)-1], "-") + } + } + cs := strings.Split(cond, separator) switch len(cs) { case 1: - params = append(params, &Parameter{Name: cond, Value: cond}) + if firstParameterCond != "" { + return nil, nil, fmt.Errorf("failed to parse params from %q: a non-parametrized component %q was found after a parametrized one %q", name, cond, firstParameterCond) + } + nameComponents = append(nameComponents, cond) case 2: + if firstParameterCond == "" { + firstParameterCond = cond + } params = append(params, &Parameter{Name: cs[0], Value: cs[1]}) default: - return nil, fmt.Errorf("failed to parse param: %s", cond) + return nil, nil, fmt.Errorf("failed to parse params from %q: %s", name, cond) } } - return params, nil + if goMaxProcs != nil { + // GOMAXPROCS should always be last in order to match the ordering of the + // benchmark name. + params = append(params, goMaxProcs) + } + return nameComponents, params, nil } // ReportCustomMetric reports a metric in a set format for parsing. @@ -93,9 +147,52 @@ func ParseCustomMetric(value, metric string) (*Metric, error) { if err != nil { return nil, fmt.Errorf("failed to parse value: %v", err) } - nameUnit := strings.Split(metric, ".") - if len(nameUnit) != 2 { - return nil, fmt.Errorf("failed to parse metric: %s", metric) + separators := []rune{'-', '.'} + var separator string + for _, sep := range separators { + if strings.ContainsRune(metric, sep) { + if separator != "" { + return nil, fmt.Errorf("failed to parse metric: ambiguous unit separator: %q (is the separator %q or %q?)", metric, separator, string(sep)) + } + separator = string(sep) + } } - return &Metric{Name: nameUnit[0], Unit: nameUnit[1], Sample: sample}, nil + var name, unit string + switch separator { + case "": + unit = metric + default: + components := strings.Split(metric, separator) + name, unit = strings.Join(components[:len(components)-1], ""), components[len(components)-1] + } + // Normalize some unit names to benchstat defaults. + switch unit { + case "": + return nil, fmt.Errorf("failed to parse metric %q: no unit specified", metric) + case "s": + unit = "sec" + case "nanos": + unit = "ns" + case "byte": + unit = "B" + case "bit": + unit = "b" + default: + // Otherwise, leave unit as-is. + } + // If the metric name is unspecified, it can sometimes be inferred from + // the unit. + if name == "" { + switch unit { + case "sec": + name = "duration" + case "req/sec", "tok/sec": + name = "throughput" + case "B/sec": + name = "bandwidth" + default: + return nil, fmt.Errorf("failed to parse metric %q: ambiguous metric name, please format the unit as 'name.unit' or 'name-unit'", metric) + } + } + return &Metric{Name: name, Unit: unit, Sample: sample}, nil } diff --git a/test/kubernetes/benchmetric/benchmetric.go b/test/kubernetes/benchmetric/benchmetric.go index 3f6641442..2281b5dcc 100644 --- a/test/kubernetes/benchmetric/benchmetric.go +++ b/test/kubernetes/benchmetric/benchmetric.go @@ -148,7 +148,7 @@ func Count(numberOfTimes uint64, thingBeingCounted string) MetricValue { if !strings.HasSuffix(thingBeingCounted, "s") { panic("`thingBeingCounted` must be plural") } - return value(float64(numberOfTimes), thingBeingCounted) + return value(float64(numberOfTimes), fmt.Sprintf("%s-num", thingBeingCounted)) } // Checksum is a MetricValue for a checksum that is not expected to change diff --git a/tools/bigquery/bigquery.go b/tools/bigquery/bigquery.go index 21ffb71bf..ec27fc0a5 100644 --- a/tools/bigquery/bigquery.go +++ b/tools/bigquery/bigquery.go @@ -85,9 +85,12 @@ func (s *Suite) debugString(sb *strings.Builder, prefix string) { // Benchstat returns a benchstat-formatted output string. // See https://pkg.go.dev/golang.org/x/perf/cmd/benchstat -// `includeConditions` contains names of `Condition`s that should be included -// as part of the benchmark name. -func (s *Suite) Benchstat(includeConditions []string) string { +// `includeCondition` returns whether a `Condition` name should be included +// as part of the benchmark name. If nil, all conditions are included. +func (s *Suite) Benchstat(includeCondition func(string) bool) string { + if includeCondition == nil { + includeCondition = func(string) bool { return true } + } var sb strings.Builder benchmarkNames := make([]string, 0, len(s.Benchmarks)) benchmarks := make(map[string]*Benchmark, len(s.Benchmarks)) @@ -98,12 +101,8 @@ func (s *Suite) Benchstat(includeConditions []string) string { } } sort.Strings(benchmarkNames) - includeConditionsMap := make(map[string]bool, len(includeConditions)) - for _, condName := range includeConditions { - includeConditionsMap[condName] = true - } for _, bmName := range benchmarkNames { - benchmarks[bmName].benchstat(&sb, s.Name, includeConditionsMap, s.Conditions) + benchmarks[bmName].benchstat(&sb, s.Name, includeCondition, s.Conditions) } return sb.String() } @@ -153,20 +152,20 @@ func noSpace(s string) string { } // benchstat produces benchmark-formatted output for this Benchmark. -func (bm *Benchmark) benchstat(sb *strings.Builder, suiteName string, includeConditions map[string]bool, suiteConditions []*Condition) { +func (bm *Benchmark) benchstat(sb *strings.Builder, suiteName string, includeCondition func(string) bool, suiteConditions []*Condition) { var conditionsStr string conditionNames := make([]string, 0, len(suiteConditions)+len(bm.Condition)) conditionMap := make(map[string]string, len(suiteConditions)+len(bm.Condition)) for _, c := range suiteConditions { cName := noSpace(c.Name) - if _, found := conditionMap[cName]; !found && includeConditions[cName] { + if _, found := conditionMap[cName]; !found && includeCondition(cName) { conditionNames = append(conditionNames, cName) conditionMap[cName] = noSpace(c.Value) } } for _, c := range bm.Condition { cName := noSpace(c.Name) - if _, found := conditionMap[cName]; !found && includeConditions[cName] { + if _, found := conditionMap[cName]; !found && includeCondition(cName) { conditionNames = append(conditionNames, cName) conditionMap[cName] = noSpace(c.Value) } diff --git a/tools/parsers/go_parser.go b/tools/parsers/go_parser.go index cef869416..d57a361fe 100644 --- a/tools/parsers/go_parser.go +++ b/tools/parsers/go_parser.go @@ -53,8 +53,8 @@ func ParseOutput(output string, name string, official bool) (*bigquery.Suite, er // *bigquery.Benchmark{ // Name: BenchmarkRuby // []*bigquery.Condition{ -// {Name: GOMAXPROCS, 6} // {Name: server_threads, 1} +// {Name: GOMAXPROCS, 6} // } // []*bigquery.Metric{ // {Name: ns/op, Unit: ns/op, Sample: 1397875880} @@ -74,12 +74,17 @@ func parseLine(line string) (*bigquery.Benchmark, error) { return nil, fmt.Errorf("expecting number of runs, got %s: %v", fields[1], err) } - name, params, err := parseNameParams(fields[0]) + nameComponents, params, err := tools.NameToParameters(fields[0]) if err != nil { return nil, fmt.Errorf("parse name/params: %v", err) } - bm := bigquery.NewBenchmark(name, iters) + // Treat the first name component as the benchmark name, and all other + // components as conditions with key = value. + bm := bigquery.NewBenchmark(nameComponents[0], iters) + for _, c := range nameComponents[1:] { + bm.AddCondition(c, c) + } for _, p := range params { bm.AddCondition(p.Name, p.Value) } @@ -94,40 +99,6 @@ func parseLine(line string) (*bigquery.Benchmark, error) { return bm, nil } -// parseNameParams parses the Name, GOMAXPROCS, and Params from the test. -// Field here should be of the format TESTNAME/PARAMS-GOMAXPROCS. -// Parameters will be separated by a "/" with individual params being -// "name.value". -func parseNameParams(field string) (string, []*tools.Parameter, error) { - var params []*tools.Parameter - // Remove GOMAXPROCS from end. - maxIndex := strings.LastIndex(field, "-") - if maxIndex < 0 { - return "", nil, fmt.Errorf("GOMAXPROCS not found: %s", field) - } - maxProcs := field[maxIndex+1:] - params = append(params, &tools.Parameter{ - Name: "GOMAXPROCS", - Value: maxProcs, - }) - - remainder := field[0:maxIndex] - index := strings.Index(remainder, "/") - if index == -1 { - return remainder, params, nil - } - - name := remainder[0:index] - p := remainder[index+1:] - - ps, err := tools.NameToParameters(p) - if err != nil { - return "", nil, fmt.Errorf("NameToParameters %s: %v", field, err) - } - params = append(params, ps...) - return name, params, nil -} - // makeMetric parses metrics and adds them to the passed Benchmark. func makeMetric(bm *bigquery.Benchmark, value, metric string) error { switch metric { diff --git a/tools/parsers/go_parser_test.go b/tools/parsers/go_parser_test.go index 39a13b4af..84fae0581 100644 --- a/tools/parsers/go_parser_test.go +++ b/tools/parsers/go_parser_test.go @@ -23,9 +23,10 @@ import ( func TestParseLine(t *testing.T) { testCases := []struct { - name string - data string - want *bigquery.Benchmark + name string + data string + want *bigquery.Benchmark + wantErr bool }{ { name: "Iperf", @@ -37,14 +38,14 @@ func TestParseLine(t *testing.T) { Name: "iterations", Value: "1", }, - { - Name: "GOMAXPROCS", - Value: "6", - }, { Name: "Upload", Value: "Upload", }, + { + Name: "GOMAXPROCS", + Value: "6", + }, }, Metric: []*bigquery.Metric{ { @@ -70,14 +71,14 @@ func TestParseLine(t *testing.T) { Name: "iterations", Value: "1", }, - { - Name: "GOMAXPROCS", - Value: "6", - }, { Name: "server_threads", Value: "1", }, + { + Name: "GOMAXPROCS", + Value: "6", + }, }, Metric: []*bigquery.Metric{ { @@ -87,7 +88,7 @@ func TestParseLine(t *testing.T) { }, { Name: "average_latency", - Unit: "s", + Unit: "sec", Sample: 0.00710, }, { @@ -98,16 +99,103 @@ func TestParseLine(t *testing.T) { }, }, }, + { + name: "Ruby with alternate parameter syntax", + data: "BenchmarkRuby/SubTest/server_threads=1/clients=8-6 1 1397875880 ns/op 0.00710 average_latency.s 140 requests_per_second.QPS", + want: &bigquery.Benchmark{ + Name: "BenchmarkRuby", + Condition: []*bigquery.Condition{ + { + Name: "iterations", + Value: "1", + }, + { + Name: "SubTest", + Value: "SubTest", + }, + { + Name: "server_threads", + Value: "1", + }, + { + Name: "clients", + Value: "8", + }, + { + Name: "GOMAXPROCS", + Value: "6", + }, + }, + Metric: []*bigquery.Metric{ + { + Name: "ns/op", + Unit: "ns/op", + Sample: 1397875880.0, + }, + { + Name: "average_latency", + Unit: "sec", + Sample: 0.00710, + }, + { + Name: "requests_per_second", + Unit: "QPS", + Sample: 140.0, + }, + }, + }, + }, + { + name: "No GOMAXPROCS is allowed", + data: "BenchmarkRuby/server_threads.1/clients.8 1 1397875880 ns/op 0.00710 average_latency.s", + want: &bigquery.Benchmark{ + Name: "BenchmarkRuby", + Condition: []*bigquery.Condition{ + { + Name: "iterations", + Value: "1", + }, + { + Name: "server_threads", + Value: "1", + }, + { + Name: "clients", + Value: "8", + }, + }, + Metric: []*bigquery.Metric{ + { + Name: "ns/op", + Unit: "ns/op", + Sample: 1397875880.0, + }, + { + Name: "average_latency", + Unit: "sec", + Sample: 0.00710, + }, + }, + }, + }, + { + name: "Ambiguous parameter separator", + data: "BenchmarkRuby/server_threads.4/clients=8 1 1397875880 ns/op 0.00710 average_latency.s", + wantErr: true, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { got, err := parseLine(tc.data) - if err != nil { + if err != nil && !tc.wantErr { t.Fatalf("parseLine failed with: %v", err) } + if err == nil && tc.wantErr { + t.Fatal("parseLine unexpectedly succeeded") + } - if !cmp.Equal(tc.want, got, nil) { + if err == nil && !cmp.Equal(tc.want, got, nil) { for i := range got.Condition { t.Logf("Metric: want: %+v got:%+v", got.Condition[i], tc.want.Condition[i]) } @@ -146,13 +234,13 @@ func TestParseOutput(t *testing.T) { { name: "Ruby", data: `BenchmarkRuby -BenchmarkRuby/server_threads.1 -BenchmarkRuby/server_threads.1-6 1 1397875880 ns/op 0.00710 average_latency.s 140 requests_per_second.QPS -BenchmarkRuby/server_threads.5 -BenchmarkRuby/server_threads.5-6 1 1416003331 ns/op 0.00950 average_latency.s 465 requests_per_second.QPS`, +BenchmarkRuby/server_threads=1 +BenchmarkRuby/server_threads=1 1 1397875880 ns/op 0.00710 average_latency.s 140 requests_per_second.QPS +BenchmarkRuby/server_threads=5 +BenchmarkRuby/server_threads=5 1 1416003331 ns/op 0.00950 average_latency.s 465 requests_per_second.QPS`, numBenchmarks: 2, numMetrics: 3, - numConditions: 3, + numConditions: 2, }, }