benchseries: ignore results with mismatched compare value

Currently, benchseries assumes that results with compare value ==
denominator is in the denominator set, and everything else is in the
numerator.

In addition to being confusing (BuilderOptions.Numerator is completely
ignored), this breaks things when the compare key isn't set at all. In
that case cmpCfg.StringValues() is always "", so everything ends up in
the numerator and nothing in the denominator, which causes a later crash
due to a missing denominator [1].

Be more explicit and only save results that explicitly fall into the
numerator or denominator.

[1] This fix isn't perfect, as benchmark results that contain only
numerator results and no denominator results will still cause a crash.
That should probably be an error or simply result in no comparisons.

For golang/go#61362.

Change-Id: I1444d0fcb101bcea0f1cdf31de7292211dcbc901
Reviewed-on: https://go-review.googlesource.com/c/perf/+/510475
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: David Chase <drchase@google.com>

Author: prattmic

benchseries/benchseries.go

@@ -362,27 +362,33 @@ func (b *Builder) Add(result *benchfmt.Result) {
 		newCell := func() *Cell {
 			return &Cell{Residues: make(map[benchproc.Key]struct{})}
 		}
-		if cmpCfg.StringValues() == b.denCompareVal {
+		switch cmpCfg.StringValues() {
+		case b.denCompareVal:
 			c = t.baseline
 			if c == nil {
 				c = newCell()
 				t.baseline = c
 				t.baselineHash = denHashCfg
 				t.baselineHashString = denHashCfg.StringValues()
 			}
-		} else {
+		case b.numCompareVal:
 			c = t.tests[numHashCfg]
 			if c == nil {
 				c = newCell()
 				t.tests[numHashCfg] = c
 				b.hashToOrder[numHashCfg] = serCfg
 			}
+		default:
+			// Compare field is set to an unknown value or
+			// completely unset. Don't add cell.
 		}
 
 		// Add to the cell.
-		c.Values = append(c.Values, result.Values[unitI].Value)
-		c.Residues[residueCfg] = struct{}{}
-		b.Residues[residueCfg] = struct{}{}
+		if c != nil {
+			c.Values = append(c.Values, result.Values[unitI].Value)
+			c.Residues[residueCfg] = struct{}{}
+			b.Residues[residueCfg] = struct{}{}
+		}
 	}
 }
 

benchseries/benchseries_test.go

@@ -0,0 +1,189 @@
+package benchseries
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+
+	"golang.org/x/perf/benchfmt"
+)
+
+func makeConfig(key, value string) benchfmt.Config {
+	return benchfmt.Config{
+		Key:   key,
+		Value: []byte(value),
+		File:  true, // N.B. Residue ignores non-File config!
+	}
+}
+
+func TestBasic(t *testing.T) {
+	results := []*benchfmt.Result{
+		{
+			Config: []benchfmt.Config{
+				makeConfig("compare", "numerator"),
+				makeConfig("runstamp", "2020-01-01T00:00:00Z"),
+				makeConfig("numerator-hash", "abcdef0123456789"),
+				makeConfig("denominator-hash", "9876543219fedcba"),
+				makeConfig("numerator-hash-time", "2020-02-02T00:00:00Z"),
+				makeConfig("residue", "foo"),
+			},
+			Name:  []byte("Foo"),
+			Iters: 10,
+			Values: []benchfmt.Value{
+				{Value: 1, Unit: "sec"},
+			},
+		},
+		{
+			Config: []benchfmt.Config{
+				makeConfig("compare", "denominator"),
+				makeConfig("runstamp", "2020-01-01T00:00:00Z"),
+				makeConfig("numerator-hash", "abcdef0123456789"),
+				makeConfig("denominator-hash", "9876543219fedcba"),
+				makeConfig("numerator-hash-time", "2020-02-02T00:00:00Z"),
+				makeConfig("residue", "foo"),
+			},
+			Name:  []byte("Foo"),
+			Iters: 10,
+			Values: []benchfmt.Value{
+				{Value: 10, Unit: "sec"},
+			},
+		},
+	}
+
+	opts := &BuilderOptions{
+		Filter:          ".unit:/.*/",
+		Series:          "numerator-hash-time",
+		Table:           "", // .unit only
+		Experiment:      "runstamp",
+		Compare:         "compare",
+		Numerator:       "numerator",
+		Denominator:     "denominator",
+		NumeratorHash:   "numerator-hash",
+		DenominatorHash: "denominator-hash",
+		Warn: func(format string, args ...interface{}) {
+			s := fmt.Sprintf(format, args...)
+			t.Errorf("benchseries warning: %s", s)
+		},
+	}
+	builder, err := NewBuilder(opts)
+	if err != nil {
+		t.Fatalf("NewBuilder get err %v, want err nil", err)
+	}
+
+	for _, r := range results {
+		builder.Add(r)
+	}
+
+	comparisons, err := builder.AllComparisonSeries(nil, DUPE_REPLACE)
+	if err != nil {
+		t.Fatalf("AllComparisonSeries got err %v want err nil", err)
+	}
+
+	if len(comparisons) != 1 {
+		t.Fatalf("len(comparisons) got %d want 1; comparisons: %+v", len(comparisons), comparisons)
+	}
+
+	got := comparisons[0]
+
+	if got.Unit != "sec" {
+		t.Errorf(`Unit got %q, want "sec"`, got.Unit)
+	}
+
+	wantBenchmarks := []string{"Foo"}
+	if !reflect.DeepEqual(got.Benchmarks, wantBenchmarks) {
+		t.Errorf("Benchmarks got %v want %v", got.Benchmarks, wantBenchmarks)
+	}
+
+	wantSeries := []string{"2020-02-02T00:00:00+00:00"}
+	if !reflect.DeepEqual(got.Series, wantSeries) {
+		t.Errorf("Series got %v want %v", got.Series, wantSeries)
+	}
+
+	wantHashPairs := map[string]ComparisonHashes{
+		"2020-02-02T00:00:00+00:00": ComparisonHashes{
+			NumHash: "abcdef0123456789",
+			DenHash: "9876543219fedcba",
+		},
+	}
+	if !reflect.DeepEqual(got.HashPairs, wantHashPairs) {
+		t.Errorf("HashPairs got %+v want %+v", got.HashPairs, wantHashPairs)
+	}
+
+	wantResidues := []StringAndSlice{{S: "residue", Slice: []string{"foo"}}}
+	if !reflect.DeepEqual(got.Residues, wantResidues) {
+		t.Errorf("Residues got %v want %v", got.Residues, wantResidues)
+	}
+}
+
+// If the compare key is missing, then there is nothing to compare.
+func TestMissingCompare(t *testing.T) {
+	results := []*benchfmt.Result{
+		{
+			Config: []benchfmt.Config{
+				makeConfig("runstamp", "2020-01-01T00:00:00Z"),
+				makeConfig("numerator-hash", "abcdef0123456789"),
+				makeConfig("denominator-hash", "9876543219fedcba"),
+				makeConfig("numerator-hash-time", "2020-02-02T00:00:00Z"),
+				makeConfig("residue", "foo"),
+			},
+			Name:  []byte("Foo"),
+			Iters: 10,
+			Values: []benchfmt.Value{
+				{Value: 1, Unit: "sec"},
+			},
+		},
+		{
+			Config: []benchfmt.Config{
+				makeConfig("runstamp", "2020-01-01T00:00:00Z"),
+				makeConfig("numerator-hash", "abcdef0123456789"),
+				makeConfig("denominator-hash", "9876543219fedcba"),
+				makeConfig("numerator-hash-time", "2020-02-02T00:00:00Z"),
+				makeConfig("residue", "foo"),
+			},
+			Name:  []byte("Foo"),
+			Iters: 10,
+			Values: []benchfmt.Value{
+				{Value: 10, Unit: "sec"},
+			},
+		},
+	}
+
+	opts := &BuilderOptions{
+		Filter:          ".unit:/.*/",
+		Series:          "numerator-hash-time",
+		Table:           "", // .unit only
+		Experiment:      "runstamp",
+		Compare:         "compare",
+		Numerator:       "numerator",
+		Denominator:     "denominator",
+		NumeratorHash:   "numerator-hash",
+		DenominatorHash: "denominator-hash",
+		Warn: func(format string, args ...interface{}) {
+			s := fmt.Sprintf(format, args...)
+			t.Errorf("benchseries warning: %s", s)
+		},
+	}
+	builder, err := NewBuilder(opts)
+	if err != nil {
+		t.Fatalf("NewBuilder get err %v, want err nil", err)
+	}
+
+	for _, r := range results {
+		builder.Add(r)
+	}
+
+	comparisons, err := builder.AllComparisonSeries(nil, DUPE_REPLACE)
+	if err != nil {
+		t.Fatalf("AllComparisonSeries got err %v want err nil", err)
+	}
+
+	if len(comparisons) != 1 {
+		t.Fatalf("len(comparisons) got %d want 1; comparisons: %+v", len(comparisons), comparisons)
+	}
+
+	got := comparisons[0]
+
+	if len(got.Series) != 0 {
+		t.Errorf("len(Series) got %d want 0; Series: %+v", len(got.Series), got.Series)
+	}
+}