Go benchstat parser: Support alternate syntax for parameters.

This supports benchmark names where parameter-value pairs are separated
by `=` rather than `.`, and benchmarks where `GOMAXPROCS` is not appended
to the name of the benchmark.

This is used in Kubernetes benchmarks where the `GOMAXPROCS` value of the
machine running the Kubernetes client has no bearing on the benchmark's
performance.

This also moves the logic for how to handle sub-test names to the caller.
Docker benchmarks continue to have the behavior of treating sub-names
as a `Condition` with key equal to its value. Kubernetes benchmarks will
instead treat sub-test names as a single `Condition` called `subtest`.

PiperOrigin-RevId: 709861103

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
 }

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

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)
 		}

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 {

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,
 		},
 	}