From ad162f40a814097f8f6028f7a82f861824596640 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesper=20Lindstr=C3=B8m?= Date: Mon, 28 Feb 2022 13:59:48 +0100 Subject: [PATCH 1/9] Optimize Combine for all nil scenarios --- benchmarks_test.go | 39 +++++++++++++++++++++++++++++++++++++++ error.go | 27 ++++++++++++++++++++++++++- error_test.go | 8 ++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/benchmarks_test.go b/benchmarks_test.go index 797f5a0..afd31f0 100644 --- a/benchmarks_test.go +++ b/benchmarks_test.go @@ -60,3 +60,42 @@ 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", func(b *testing.B) { + var x, y, z error + for i := 0; i < b.N; i++ { + Combine(x, y, z) + } + }) + + b.Run("inline 3 with 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("slice", func(b *testing.B) { + var x, y, z error + for i := 0; i < b.N; i++ { + errs := []error{x, y, z} + Combine(errs...) + } + }) +} diff --git a/error.go b/error.go index 13a020a..ac6713f 100644 --- a/error.go +++ b/error.go @@ -434,7 +434,32 @@ 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: + wasNil := true + for _, err := range errors { + if err != nil { + wasNil = false + break + } + } + if wasNil { + return nil + } + + // If we don't copy the errors slice the escape analysis will mark errors + // and it will always be allocated on the heap. + errs := make([]error, len(errors)) + copy(errs, errors) + + return fromSlice(errs) + } } // Append appends the given errors together. Either value may be nil. diff --git a/error_test.go b/error_test.go index 3dd4884..de44d2e 100644 --- a/error_test.go +++ b/error_test.go @@ -273,6 +273,14 @@ func TestCombineDoesNotModifySlice(t *testing.T) { assert.Nil(t, errors[1], 3) } +func TestCombineGoodCaseNoAlloc(t *testing.T) { + var x, y, z error + allocs := testing.AllocsPerRun(100, func() { + Combine(x, y, z) + }) + assert.Equal(t, 0.0, allocs) +} + func TestAppend(t *testing.T) { tests := []struct { left error From 3d2b5ea3f984348f6869b9c1d2f3d75fff9b53e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesper=20Lindstr=C3=B8m?= Date: Mon, 28 Feb 2022 14:24:12 +0100 Subject: [PATCH 2/9] optimize for one error --- benchmarks_test.go | 34 ++++++++++++++++++++++++++++++---- error.go | 25 ++++++++++++++++++------- error_test.go | 6 ++++++ 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/benchmarks_test.go b/benchmarks_test.go index afd31f0..ec694fb 100644 --- a/benchmarks_test.go +++ b/benchmarks_test.go @@ -76,14 +76,14 @@ func BenchmarkCombine(b *testing.B) { } }) - b.Run("inline 3", func(b *testing.B) { + 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 with error", func(b *testing.B) { + 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++ { @@ -91,10 +91,36 @@ func BenchmarkCombine(b *testing.B) { } }) - b.Run("slice", func(b *testing.B) { + 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++ { - errs := []error{x, y, z} + 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...) } }) diff --git a/error.go b/error.go index ac6713f..89a14fa 100644 --- a/error.go +++ b/error.go @@ -442,21 +442,32 @@ func Combine(errors ...error) error { case 2: return Append(errors[0], errors[1]) default: - wasNil := true - for _, err := range errors { + idx := -1 + onlyOne := false + // wasNil := true + for i, err := range errors { if err != nil { - wasNil = false - break + if onlyOne { + onlyOne = false + break + } + onlyOne = true + idx = i } } - if wasNil { + + if idx == -1 { return nil } + if onlyOne { + return errors[idx] + } + // If we don't copy the errors slice the escape analysis will mark errors // and it will always be allocated on the heap. - errs := make([]error, len(errors)) - copy(errs, errors) + errs := make([]error, len(errors)-idx) + copy(errs, errors[idx:]) return fromSlice(errs) } diff --git a/error_test.go b/error_test.go index de44d2e..3811e8b 100644 --- a/error_test.go +++ b/error_test.go @@ -281,6 +281,12 @@ func TestCombineGoodCaseNoAlloc(t *testing.T) { assert.Equal(t, 0.0, allocs) } +func TestOptimize(t *testing.T) { + errs := make([]error, 100) + errs[len(errs)-1] = fmt.Errorf("failed") + Combine(errs...) +} + func TestAppend(t *testing.T) { tests := []struct { left error From 6acb82dba59c80cef088fcb974876bf62200b252 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesper=20Lindstr=C3=B8m?= Date: Mon, 28 Feb 2022 14:34:17 +0100 Subject: [PATCH 3/9] Removed unused code --- error.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/error.go b/error.go index 89a14fa..0e5a23c 100644 --- a/error.go +++ b/error.go @@ -444,7 +444,6 @@ func Combine(errors ...error) error { default: idx := -1 onlyOne := false - // wasNil := true for i, err := range errors { if err != nil { if onlyOne { @@ -464,8 +463,8 @@ func Combine(errors ...error) error { return errors[idx] } - // If we don't copy the errors slice the escape analysis will mark errors - // and it will always be allocated on the heap. + // 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:]) From 22c7808df70247848ab89b3f8d58e61393571527 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesper=20Lindstr=C3=B8m?= Date: Mon, 28 Feb 2022 15:16:42 +0100 Subject: [PATCH 4/9] cleanup allocation tests --- error_test.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/error_test.go b/error_test.go index 3811e8b..31c8c5c 100644 --- a/error_test.go +++ b/error_test.go @@ -274,19 +274,13 @@ func TestCombineDoesNotModifySlice(t *testing.T) { } func TestCombineGoodCaseNoAlloc(t *testing.T) { - var x, y, z error + errs := make([]error, 10) allocs := testing.AllocsPerRun(100, func() { - Combine(x, y, z) + Combine(errs...) }) assert.Equal(t, 0.0, allocs) } -func TestOptimize(t *testing.T) { - errs := make([]error, 100) - errs[len(errs)-1] = fmt.Errorf("failed") - Combine(errs...) -} - func TestAppend(t *testing.T) { tests := []struct { left error From 9a9809b35eb1ce50a627c4775988f04684aab32d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 28 Feb 2022 07:49:07 -0800 Subject: [PATCH 5/9] bench: Don't allocate slices inside the loop --- benchmarks_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/benchmarks_test.go b/benchmarks_test.go index ec694fb..562d3bc 100644 --- a/benchmarks_test.go +++ b/benchmarks_test.go @@ -102,25 +102,25 @@ func BenchmarkCombine(b *testing.B) { }) b.Run("slice 100 no errors", func(b *testing.B) { + errs := make([]error, 100) for i := 0; i < b.N; i++ { - errs := make([]error, 100) 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++ { - errs := make([]error, 100) - errs[len(errs)-1] = fmt.Errorf("failed") 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++ { - errs := make([]error, 100) - errs[0] = fmt.Errorf("failed1") - errs[len(errs)-1] = fmt.Errorf("failed2") Combine(errs...) } }) From 2ca2005103b32bfd594bf24f6293aff556cd53b5 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 28 Feb 2022 08:03:33 -0800 Subject: [PATCH 6/9] Simplify solution --- error.go | 45 +++++++-------------------------------------- 1 file changed, 7 insertions(+), 38 deletions(-) diff --git a/error.go b/error.go index 0e5a23c..882c7b2 100644 --- a/error.go +++ b/error.go @@ -381,8 +381,12 @@ 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. + out := make([]error, len(errors)) + copy(out, errors) + return &multiError{errors: out} } } @@ -434,42 +438,7 @@ func fromSlice(errors []error) error { // // fmt.Sprintf("%+v", multierr.Combine(err1, err2)) func Combine(errors ...error) error { - 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) - } + return fromSlice(errors) } // Append appends the given errors together. Either value may be nil. From b63bdaac7a2d0f4cfa62ee467e2884b2383281f0 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 28 Feb 2022 08:17:49 -0800 Subject: [PATCH 7/9] Don't inspect slices of 0 or 1 --- error.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/error.go b/error.go index 882c7b2..fd6bb64 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: From 5842d36559d14105312786787e9c309b36c3ceb4 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 28 Feb 2022 08:28:16 -0800 Subject: [PATCH 8/9] test/cover: Slice with a single non-nil error --- error_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/error_test.go b/error_test.go index 31c8c5c..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"), From c5a74ae3bb9979a08807a1d640e32c714cb28b3a Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 28 Feb 2022 08:29:21 -0800 Subject: [PATCH 9/9] better explain the comment about copying the slice --- error.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/error.go b/error.go index fd6bb64..f45af14 100644 --- a/error.go +++ b/error.go @@ -389,9 +389,10 @@ func fromSlice(errors []error) error { return errors[res.FirstErrorIdx] case len(errors): if !res.ContainsMultiError { - // Error list is flat. - // Make a copy of it (otherwise "errors" escapes to the - // heap) unconditionally for all other cases. + // 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}