Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Combine for all nil scenarios #55

Merged
merged 9 commits into from Feb 28, 2022
Merged
65 changes: 65 additions & 0 deletions benchmarks_test.go
Expand Up @@ -60,3 +60,68 @@ func BenchmarkAppend(b *testing.B) {
}
}
}

func BenchmarkCombine(b *testing.B) {
b.Run("inline 1", func(b *testing.B) {
var x error
for i := 0; i < b.N; i++ {
Combine(x)
}
})

b.Run("inline 2", func(b *testing.B) {
var x, y error
for i := 0; i < b.N; i++ {
Combine(x, y)
}
})

b.Run("inline 3 no error", func(b *testing.B) {
var x, y, z error
for i := 0; i < b.N; i++ {
Combine(x, y, z)
}
})

b.Run("inline 3 one error", func(b *testing.B) {
var x, y, z error
z = fmt.Errorf("failed")
for i := 0; i < b.N; i++ {
Combine(x, y, z)
}
})

b.Run("inline 3 multiple errors", func(b *testing.B) {
var x, y, z error
z = fmt.Errorf("failed3")
y = fmt.Errorf("failed2")
x = fmt.Errorf("failed")
for i := 0; i < b.N; i++ {
Combine(x, y, z)
}
})

b.Run("slice 100 no errors", func(b *testing.B) {
for i := 0; i < b.N; i++ {
errs := make([]error, 100)
Combine(errs...)
}
})

b.Run("slice 100 one error", func(b *testing.B) {
for i := 0; i < b.N; i++ {
errs := make([]error, 100)
errs[len(errs)-1] = fmt.Errorf("failed")
Combine(errs...)
}
})

b.Run("slice 100 multi error", func(b *testing.B) {
for i := 0; i < b.N; i++ {
errs := make([]error, 100)
errs[0] = fmt.Errorf("failed1")
errs[len(errs)-1] = fmt.Errorf("failed2")
Combine(errs...)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These benchmarks probably shouldn't allocate the test slices inside the loop?
Fixing.

}
})
}
37 changes: 36 additions & 1 deletion error.go
Expand Up @@ -434,7 +434,42 @@ func fromSlice(errors []error) error {
//
// fmt.Sprintf("%+v", multierr.Combine(err1, err2))
func Combine(errors ...error) error {
return fromSlice(errors)
switch len(errors) {
case 0:
return nil
case 1:
return errors[0]
case 2:
return Append(errors[0], errors[1])
default:
idx := -1
onlyOne := false
for i, err := range errors {
if err != nil {
if onlyOne {
onlyOne = false
break
}
onlyOne = true
idx = i
}
}

if idx == -1 {
return nil
}

if onlyOne {
return errors[idx]
}

// If we don't copy the errors slice here the escape analysis will mark
// it, which will result in a heap allocation on every call.
errs := make([]error, len(errors)-idx)
copy(errs, errors[idx:])

return fromSlice(errs)
}
}

// Append appends the given errors together. Either value may be nil.
Expand Down
8 changes: 8 additions & 0 deletions error_test.go
Expand Up @@ -273,6 +273,14 @@ func TestCombineDoesNotModifySlice(t *testing.T) {
assert.Nil(t, errors[1], 3)
}

func TestCombineGoodCaseNoAlloc(t *testing.T) {
errs := make([]error, 10)
allocs := testing.AllocsPerRun(100, func() {
Combine(errs...)
})
assert.Equal(t, 0.0, allocs)
}

func TestAppend(t *testing.T) {
tests := []struct {
left error
Expand Down