Avoid allocating a new slice on every Append (#9)
Currently, appending a non-nil error to a multierror always copies the underlying slice, since there may be multiple goroutines appending to the same multierr,
```go
merr := multierr.Append(errors.New("initial"), errors.New("multierr"))
go func() {
err1 = multierr.Append(merr, errors.New("1"))
[..]
}()
go func() {
err2 = multierr.Append(merr, errors.New("2"))
[..]
}()
```
However, the common use case is a standard for loop:
```go
var merr error
for _, v := range values {
merr = multierr.Append(merr, processValue(v))
}
return merr
```
Since there is only a single resulting slice, we don't need to create a full copy on every `Append`. Instead, we track whether we have modified the underlying slice from `Append` already using an atomic boolean, and only create a copy if the underlying slice has been modified.
diff --git a/error.go b/error.go
index a247250..c6a9b75 100644
--- a/error.go
+++ b/error.go
@@ -27,6 +27,8 @@
"io"
"strings"
"sync"
+
+ "go.uber.org/atomic"
)
var (
@@ -68,13 +70,12 @@
//
// multiError formats to a semi-colon delimited list of error messages with
// %v and with a more readable multi-line format with %+v.
-type multiError []error
-
-func (merr multiError) String() string {
- return merr.Error()
+type multiError struct {
+ copyNeeded atomic.Bool
+ errors []error
}
-func (merr multiError) Error() string {
+func (merr *multiError) Error() string {
buff := _bufferPool.Get().(*bytes.Buffer)
buff.Reset()
@@ -85,7 +86,7 @@
return result
}
-func (merr multiError) Format(f fmt.State, c rune) {
+func (merr *multiError) Format(f fmt.State, c rune) {
if c == 'v' && f.Flag('+') {
merr.writeMultiline(f)
} else {
@@ -93,9 +94,9 @@
}
}
-func (merr multiError) writeSingleline(w io.Writer) {
+func (merr *multiError) writeSingleline(w io.Writer) {
first := true
- for _, item := range merr {
+ for _, item := range merr.errors {
if first {
first = false
} else {
@@ -105,9 +106,9 @@
}
}
-func (merr multiError) writeMultiline(w io.Writer) {
+func (merr *multiError) writeMultiline(w io.Writer) {
w.Write(_multilinePrefix)
- for _, item := range merr {
+ for _, item := range merr.errors {
w.Write(_multilineSeparator)
writePrefixLine(w, _multilineIndent, fmt.Sprintf("%+v", item))
}
@@ -164,8 +165,8 @@
res.FirstErrorIdx = i
}
- if merr, ok := err.(multiError); ok {
- res.Capacity += len(merr)
+ if merr, ok := err.(*multiError); ok {
+ res.Capacity += len(merr.errors)
res.ContainsMultiError = true
} else {
res.Capacity++
@@ -186,23 +187,24 @@
case len(errors):
if !res.ContainsMultiError {
// already flat
- return multiError(errors)
+ return &multiError{errors: errors}
}
}
- merr := make(multiError, 0, res.Capacity)
+ nonNilErrs := make([]error, 0, res.Capacity)
for _, err := range errors[res.FirstErrorIdx:] {
if err == nil {
continue
}
- if nested, ok := err.(multiError); ok {
- merr = append(merr, nested...)
+ if nested, ok := err.(*multiError); ok {
+ nonNilErrs = append(nonNilErrs, nested.errors...)
} else {
- merr = append(merr, err)
+ nonNilErrs = append(nonNilErrs, err)
}
}
- return merr
+
+ return &multiError{errors: nonNilErrs}
}
// Combine combines the passed errors into a single error.
@@ -263,10 +265,15 @@
return left
}
- if _, ok := right.(multiError); !ok {
- if _, ok := left.(multiError); !ok {
+ if _, ok := right.(*multiError); !ok {
+ if l, ok := left.(*multiError); ok && !l.copyNeeded.Swap(true) {
+ // Common case where the error on the left is constantly being
+ // appended to.
+ errs := append(l.errors, right)
+ return &multiError{errors: errs}
+ } else if !ok {
// Both errors are single errors.
- return multiError{left, right}
+ return &multiError{errors: []error{left, right}}
}
}
diff --git a/error_test.go b/error_test.go
index e54fcd7..e05b905 100644
--- a/error_test.go
+++ b/error_test.go
@@ -24,6 +24,7 @@
"errors"
"fmt"
"io"
+ "sync"
"testing"
"github.com/stretchr/testify/assert"
@@ -54,6 +55,10 @@
return errs
}
+func newMultiErr(errors ...error) error {
+ return &multiError{errors: errors}
+}
+
func TestCombine(t *testing.T) {
tests := []struct {
giveErrors []error
@@ -73,15 +78,15 @@
giveErrors: []error{
errors.New("foo"),
nil,
- multiError{
+ newMultiErr(
errors.New("bar"),
- },
+ ),
nil,
},
- wantError: multiError{
+ wantError: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
+ ),
wantMultiline: "the following errors occurred:\n" +
" - foo\n" +
" - bar",
@@ -90,14 +95,14 @@
{
giveErrors: []error{
errors.New("foo"),
- multiError{
+ newMultiErr(
errors.New("bar"),
- },
+ ),
},
- wantError: multiError{
+ wantError: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
+ ),
wantMultiline: "the following errors occurred:\n" +
" - foo\n" +
" - bar",
@@ -114,10 +119,10 @@
errors.New("foo"),
errors.New("bar"),
},
- wantError: multiError{
+ wantError: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
+ ),
wantMultiline: "the following errors occurred:\n" +
" - foo\n" +
" - bar",
@@ -129,11 +134,11 @@
errors.New("multi\n line\nerror message"),
errors.New("single line error message"),
},
- wantError: multiError{
+ wantError: newMultiErr(
errors.New("great sadness"),
errors.New("multi\n line\nerror message"),
errors.New("single line error message"),
- },
+ ),
wantMultiline: "the following errors occurred:\n" +
" - great sadness\n" +
" - multi\n" +
@@ -147,18 +152,18 @@
{
giveErrors: []error{
errors.New("foo"),
- multiError{
+ newMultiErr(
errors.New("bar"),
errors.New("baz"),
- },
+ ),
errors.New("qux"),
},
- wantError: multiError{
+ wantError: newMultiErr(
errors.New("foo"),
errors.New("bar"),
errors.New("baz"),
errors.New("qux"),
- },
+ ),
wantMultiline: "the following errors occurred:\n" +
" - foo\n" +
" - bar\n" +
@@ -170,15 +175,15 @@
giveErrors: []error{
errors.New("foo"),
nil,
- multiError{
+ newMultiErr(
errors.New("bar"),
- },
+ ),
nil,
},
- wantError: multiError{
+ wantError: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
+ ),
wantMultiline: "the following errors occurred:\n" +
" - foo\n" +
" - bar",
@@ -187,14 +192,14 @@
{
giveErrors: []error{
errors.New("foo"),
- multiError{
+ newMultiErr(
errors.New("bar"),
- },
+ ),
},
- wantError: multiError{
+ wantError: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
+ ),
wantMultiline: "the following errors occurred:\n" +
" - foo\n" +
" - bar",
@@ -206,11 +211,11 @@
richFormatError{},
errors.New("bar"),
},
- wantError: multiError{
+ wantError: newMultiErr(
errors.New("foo"),
richFormatError{},
errors.New("bar"),
- },
+ ),
wantMultiline: "the following errors occurred:\n" +
" - foo\n" +
" - multiline\n" +
@@ -277,46 +282,46 @@
{
left: errors.New("foo"),
right: errors.New("bar"),
- want: multiError{
+ want: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
+ ),
},
{
- left: multiError{
+ left: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
+ ),
right: errors.New("baz"),
- want: multiError{
+ want: newMultiErr(
errors.New("foo"),
errors.New("bar"),
errors.New("baz"),
- },
+ ),
},
{
left: errors.New("baz"),
- right: multiError{
+ right: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
- want: multiError{
+ ),
+ want: newMultiErr(
errors.New("baz"),
errors.New("foo"),
errors.New("bar"),
- },
+ ),
},
{
- left: multiError{
+ left: newMultiErr(
errors.New("foo"),
- },
- right: multiError{
+ ),
+ right: newMultiErr(
errors.New("bar"),
- },
- want: multiError{
+ ),
+ want: newMultiErr(
errors.New("foo"),
errors.New("bar"),
- },
+ ),
},
}
@@ -325,20 +330,40 @@
}
}
-func TestAppendDoesNotModify(t *testing.T) {
- createInitial := func() error {
- // Create a multiError that has capacity for more errors so Append will
- // modify the underlying array that may be shared.
- return appendN(nil, errors.New("initial"), 50)
- }
+func createMultiErrWithCapacity() error {
+ // Create a multiError that has capacity for more errors so Append will
+ // modify the underlying array that may be shared.
+ return appendN(nil, errors.New("append"), 50)
+}
- initial := createInitial()
+func TestAppendDoesNotModify(t *testing.T) {
+ initial := createMultiErrWithCapacity()
err1 := Append(initial, errors.New("err1"))
err2 := Append(initial, errors.New("err2"))
- // initial should not have been modified.
- assert.Equal(t, createInitial(), initial, "Initial should not be modified")
+ // Make sure the error messages match, since we do modify the copyNeeded
+ // atomic, the values cannot be compared.
+ assert.EqualError(t, initial, createMultiErrWithCapacity().Error(), "Initial should not be modified")
- assert.Equal(t, Append(createInitial(), errors.New("err1")), err1)
- assert.Equal(t, Append(createInitial(), errors.New("err2")), err2)
+ assert.EqualError(t, err1, Append(createMultiErrWithCapacity(), errors.New("err1")).Error())
+ assert.EqualError(t, err2, Append(createMultiErrWithCapacity(), errors.New("err2")).Error())
+}
+
+func TestAppendRace(t *testing.T) {
+ initial := createMultiErrWithCapacity()
+
+ var wg sync.WaitGroup
+ for i := 0; i < 10; i++ {
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+
+ err := initial
+ for j := 0; j < 10; j++ {
+ err = Append(err, errors.New("err"))
+ }
+ }()
+ }
+
+ wg.Wait()
}
diff --git a/glide.lock b/glide.lock
index 15c5fa4..f9ea94c 100644
--- a/glide.lock
+++ b/glide.lock
@@ -1,16 +1,19 @@
-hash: cb430176250bcfe8bafd93aa48d285fa35b08af59b5f9971dcf553a1d6c236ec
-updated: 2017-03-17T16:22:25.024129734-07:00
-imports: []
+hash: b53b5e9a84b9cb3cc4b2d0499e23da2feca1eec318ce9bb717ecf35bf24bf221
+updated: 2017-04-10T13:34:45.671678062-07:00
+imports:
+- name: go.uber.org/atomic
+ version: 3b8db5e93c4c02efbc313e17b2e796b0914a01fb
testImports:
- name: github.com/davecgh/go-spew
- version: 346938d642f2ec3594ed81d874461961cd0faa76
+ version: 6d212800a42e8ab5c146b8ace3490ee17e5225f9
subpackages:
- spew
- name: github.com/pmezard/go-difflib
- version: 792786c7400a136282c1664665ae0a8db921c6c2
+ version: d8ed2627bdf02c080bf22230dbb337003b7aba2d
subpackages:
- difflib
- name: github.com/stretchr/testify
- version: 4d4bfba8f1d1027c4fdbe371823030df51419987
+ version: 69483b4bd14f5845b5a1e55bca19e954e827f1d0
subpackages:
- assert
+ - require
diff --git a/glide.yaml b/glide.yaml
index 67951f5..6ef084e 100644
--- a/glide.yaml
+++ b/glide.yaml
@@ -1,5 +1,7 @@
package: go.uber.org/multierr
-import: []
+import:
+- package: go.uber.org/atomic
+ version: ^1
testImport:
- package: github.com/stretchr/testify
subpackages:
