Skip to content

Commit

Permalink
Allow batched diffing of slices with a custom comparer
Browse files Browse the repository at this point in the history
For correctness, cmp checks applicability of the options to every
element in a slice. For large []byte, this is a significant performance
detriment. The workaround is to specify Comparer(bytes.Equal).
However, we would still like to have the batched diffing if the
slices are different. Specialize for this situation.
  • Loading branch information
dsnet committed Jun 9, 2020
1 parent 23a2b56 commit d5c5413
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 3 deletions.
9 changes: 9 additions & 0 deletions cmp/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,15 @@ func reporterTests() []test {
y: MyComposite{IntsA: []int8{12, 29, 13, 27, 22, 23, 17, 18, 19, 20, 21, 10, 26, 16, 25, 28, 11, 15, 24, 14}},
wantEqual: false,
reason: "batched diffing desired since many elements differ",
}, {
label: label + "/BatchedWithComparer",
x: MyComposite{BytesA: []byte{10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29}},
y: MyComposite{BytesA: []byte{12, 29, 13, 27, 22, 23, 17, 18, 19, 20, 21, 10, 26, 16, 25, 28, 11, 15, 24, 14}},
wantEqual: false,
opts: []cmp.Option{
cmp.Comparer(bytes.Equal),
},
reason: "batched diffing desired since many elements differ",
}, {
label: label,
x: MyComposite{
Expand Down
18 changes: 15 additions & 3 deletions cmp/report_slices.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,25 @@ func (opts formatOptions) CanFormatDiffSlice(v *valueNode) bool {
return false // Must be formatting in diff mode
case v.NumDiff == 0:
return false // No differences detected
case v.NumIgnored+v.NumCompared+v.NumTransformed > 0:
// TODO: Handle the case where someone uses bytes.Equal on a large slice.
return false // Some custom option was used to determined equality
case !v.ValueX.IsValid() || !v.ValueY.IsValid():
return false // Both values must be valid
case v.Type.Kind() == reflect.Slice && (v.ValueX.IsNil() || v.ValueY.IsNil()):
return false // Both of values have to be non-nil
case v.NumIgnored > 0:
return false // Some ignore option was used
case v.NumTransformed > 0:
return false // Some transform option was used
case v.NumCompared > 1:
return false // More than one comparison was used
case v.NumCompared == 1 && v.Type.Name() != "":
// The need for cmp to check applicability of options on every element
// in a slice is a significant performance detriment for large []byte.
// The workaround is to specify Comparer(bytes.Equal),
// which enables cmp to compare []byte more efficiently.
// If they differ, we still want to provide batched diffing.
// The logic disallows named types since they tend to have their own
// String method, with nicer formatting than what this provides.
return false
}

switch t := v.Type; t.Kind() {
Expand Down
16 changes: 16 additions & 0 deletions cmp/testdata/diffs
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,22 @@
... // 6 identical fields
}
>>> TestDiff/Reporter#01
<<< TestDiff/Reporter/BatchedWithComparer
cmp_test.MyComposite{
StringA: "",
StringB: "",
BytesA: []uint8{
- 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, // -|.......|
+ 0x0c, 0x1d, 0x0d, 0x1b, 0x16, 0x17, // +|......|
0x11, 0x12, 0x13, 0x14, 0x15, // |.....|
- 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, // -|........|
+ 0x0a, 0x1a, 0x10, 0x19, 0x1c, 0x0b, 0x0f, 0x18, 0x0e, // +|.........|
},
BytesB: nil,
BytesC: nil,
... // 9 identical fields
}
>>> TestDiff/Reporter/BatchedWithComparer
<<< TestDiff/Reporter#02
cmp_test.MyComposite{
StringA: "",
Expand Down

0 comments on commit d5c5413

Please sign in to comment.