Optimize Combine for all nil scenarios #55
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #55 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 105 112 +7
=========================================
+ Hits 105 112 +7
Continue to review full report at Codecov.
|
benchmarks_test.go
Outdated
| 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...) |
There was a problem hiding this comment.
These benchmarks probably shouldn't allocate the test slices inside the loop?
Fixing.
|
Nice one @jstroem, but I don't think it needs to be this complicated. The simplified version performs a nanosecond slower (because it inspects unconditionally—fixable), does the same number of allocations or fewer in the case of "slice of 100 with multiple errors" (because it optimizes for the "no nested multierrors case"). |
Updated to not inspect for 0 or 1 items. We can special case 2 items as well, if we'd like but it's off by a nanosecond or less so I'm not sure how much worth it is to do that. |
During the work on yarpc/yarpc-go#2126 I found that
multierr.Combinealways allocates the error slice on the heap because of the escape analysis.Assuming that most cases where
multierr.Combineis called all arguments arenilit make sense to optimize it with that in mind:Benchmark results for this optimization: