Use standard merge algorithm when merging Values #8131
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jwilder two-way merges works best when the two lists are around the same size. Would it be unusual to merge very differently sized lists? If so, could we add a benchmark for that? |
|
@e-dard I'll add those. Sizes of both lists will usually be different. |
Merge had the side effect of modifying the original values so the results are wrong because they always hit the fast path after the first run.
jwilder
force-pushed
the
jw-values-merge
branch
from
6040e9b to
0e07a12
Compare
|
@e-dard Added some other benchmarks: |
e-dard
suggested changes
Mar 15, 2017
| if a[0].UnixNano() < b[0].UnixNano() { | ||
| out, a = append(out, a[0]), a[1:] | ||
| } else { | ||
| if len(b) > 0 && a[0].UnixNano() == b[0].UnixNano() { |
There was a problem hiding this comment.
You can move line 145 up to make 144 an else if.
| out, a = append(out, a[0]), a[1:] | ||
| } else { | ||
| if len(b) > 0 && a[0].UnixNano() == b[0].UnixNano() { | ||
| a = a[1:] | ||
| } else { |
There was a problem hiding this comment.
This else can then be an else on the outer if block, removing some indentation.
jwilder
force-pushed
the
jw-values-merge
branch
from
0e07a12 to
7561dea
Compare
e-dard
suggested changes
Mar 15, 2017
| out, b = append(out, b[0]), b[1:] | ||
| } | ||
| } | ||
| if len(a) > 0 { |
There was a problem hiding this comment.
This can be simplified to:
if len(a) > 0 {
return append(out, a...)
}
return append(out, b...)The previous version was very innefficient due to the benchmarks used to optimize it having a bug. This version always allocates a new slice, but is O(n).
jwilder
force-pushed
the
jw-values-merge
branch
from
7561dea to
a4cfeac
Compare
e-dard
approved these changes
Mar 15, 2017
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While investigating a runtime/GC stall, @aclements noticed that
Values.Mergewas using a sub-optimal algorithm and that a traditional merge algorithm would likely perform better in general. He also discovered that the benchmark we were using had a bug which caused the current implementation to appear better when it was actually quite worse in all but one case.This switches the
Values.Mergeto use a standard merge algorithm.