From cea7d2e1fc49ac9540637fdcad2744bd0cf08431 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesper=20Lindstr=C3=B8m=20Nielsen?= Date: Mon, 28 Feb 2022 21:01:14 +0100 Subject: [PATCH] Combine: Optimize for all nil (#55) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current implementation of Combine returns the input slice in the returned multierr. This causes it to escape to the heap unconditionally. Optimizing for the no-errors case, we can copy the slice in that case and keep it on the stack. ``` name old time/op new time/op delta Combine/inline_1-8 17.7ns ± 0% 2.1ns ± 0% -88.20% (p=0.008 n=5+5) Combine/inline_2-8 21.0ns ± 0% 4.4ns ± 1% -79.03% (p=0.008 n=5+5) Combine/inline_3_no_error-8 24.4ns ± 0% 5.1ns ± 1% -79.22% (p=0.016 n=4+5) Combine/inline_3_one_error-8 24.8ns ± 0% 5.4ns ± 1% -78.42% (p=0.008 n=5+5) Combine/inline_3_multiple_errors-8 44.3ns ± 0% 54.9ns ± 0% +23.80% (p=0.008 n=5+5) Combine/slice_100_no_errors-8 72.9ns ± 0% 73.4ns ± 1% +0.68% (p=0.008 n=5+5) Combine/slice_100_one_error-8 74.5ns ± 0% 74.8ns ± 0% ~ (p=0.056 n=5+5) Combine/slice_100_multi_error-8 193ns ± 0% 193ns ± 0% ~ (p=0.127 n=5+5) name old alloc/op new alloc/op delta Combine/inline_1-8 16.0B ± 0% 0.0B -100.00% (p=0.008 n=5+5) Combine/inline_2-8 32.0B ± 0% 0.0B -100.00% (p=0.008 n=5+5) Combine/inline_3_no_error-8 48.0B ± 0% 0.0B -100.00% (p=0.008 n=5+5) Combine/inline_3_one_error-8 48.0B ± 0% 0.0B -100.00% (p=0.008 n=5+5) Combine/inline_3_multiple_errors-8 80.0B ± 0% 80.0B ± 0% ~ (all equal) Combine/slice_100_no_errors-8 0.00B 0.00B ~ (all equal) Combine/slice_100_one_error-8 0.00B 0.00B ~ (all equal) Combine/slice_100_multi_error-8 64.0B ± 0% 64.0B ± 0% ~ (all equal) name old allocs/op new allocs/op delta Combine/inline_1-8 1.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) Combine/inline_2-8 1.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) Combine/inline_3_no_error-8 1.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) Combine/inline_3_one_error-8 1.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) Combine/inline_3_multiple_errors-8 2.00 ± 0% 2.00 ± 0% ~ (all equal) Combine/slice_100_no_errors-8 0.00 0.00 ~ (all equal) Combine/slice_100_one_error-8 0.00 0.00 ~ (all equal) Combine/slice_100_multi_error-8 2.00 ± 0% 2.00 ± 0% ~ (all equal) ``` Co-authored-by: Abhinav Gupta --- benchmarks_test.go | 65 ++++++++++++++++++++++++++++++++++++++++++++++ error.go | 17 ++++++++++-- error_test.go | 14 ++++++++++ 3 files changed, 94 insertions(+), 2 deletions(-) diff --git a/benchmarks_test.go b/benchmarks_test.go index 797f5a0..562d3bc 100644 --- a/benchmarks_test.go +++ b/benchmarks_test.go @@ -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) { + errs := make([]error, 100) + for i := 0; i < b.N; i++ { + Combine(errs...) + } + }) + + b.Run("slice 100 one error", func(b *testing.B) { + errs := make([]error, 100) + errs[len(errs)-1] = fmt.Errorf("failed") + for i := 0; i < b.N; i++ { + Combine(errs...) + } + }) + + b.Run("slice 100 multi error", func(b *testing.B) { + errs := make([]error, 100) + errs[0] = fmt.Errorf("failed1") + errs[len(errs)-1] = fmt.Errorf("failed2") + for i := 0; i < b.N; i++ { + Combine(errs...) + } + }) +} diff --git a/error.go b/error.go index 13a020a..f45af14 100644 --- a/error.go +++ b/error.go @@ -372,6 +372,14 @@ func inspect(errors []error) (res inspectResult) { // fromSlice converts the given list of errors into a single error. func fromSlice(errors []error) error { + // Don't pay to inspect small slices. + switch len(errors) { + case 0: + return nil + case 1: + return errors[0] + } + res := inspect(errors) switch res.Count { case 0: @@ -381,8 +389,13 @@ func fromSlice(errors []error) error { return errors[res.FirstErrorIdx] case len(errors): if !res.ContainsMultiError { - // already flat - return &multiError{errors: errors} + // Error list is flat. Make a copy of it + // Otherwise "errors" escapes to the heap + // unconditionally for all other cases. + // This lets us optimize for the "no errors" case. + out := make([]error, len(errors)) + copy(out, errors) + return &multiError{errors: out} } } diff --git a/error_test.go b/error_test.go index 3dd4884..c053167 100644 --- a/error_test.go +++ b/error_test.go @@ -97,6 +97,12 @@ func TestCombine(t *testing.T) { " - bar", wantSingleline: "foo; bar", }, + { + giveErrors: []error{nil, nil, errors.New("great sadness"), nil}, + wantError: errors.New("great sadness"), + wantMultiline: "great sadness", + wantSingleline: "great sadness", + }, { giveErrors: []error{ errors.New("foo"), @@ -273,6 +279,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