loser-tree: add sequence abstraction #376
Draft
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
Having the implementation limited to slices limits the extent to which this package can be re-used. The PR introducing loser tree stated that this was to achieve a 15% performance improvement, but benchmarks on this change give a mixed picture, with some going 5% faster and some 7% slower.
charleskorn
reviewed
Sep 6, 2023
| @@ -565,6 +565,7 @@ func TestDoUntilQuorumWithoutSuccessfulContextCancellation_PartialZoneFailure(t | |||
| } | |||
|
|
|||
| func TestDoUntilQuorumWithoutSuccessfulContextCancellation_CancelsEntireZoneImmediatelyOnSingleFailure(t *testing.T) { | |||
| t.Skip() | |||
There was a problem hiding this comment.
Did you intend to include this?
| } | ||
|
|
||
| return out | ||
| } | ||
|
|
||
| // Wrapper over a slice that implements the loser.Sequence API |
There was a problem hiding this comment.
Is it worth including a version of this in the loser package? I imagine this won't be the only place that needs to work with slices.
Having said that, I'm not sure whether we'd want a generic slice sequence type, or specialised types - would be interesting to benchmark and see if the use of generics makes any substantial performance difference.
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.
What this PR does:
Give the loser tree package a sequence abstraction.
Having the implementation limited to slices limits the extent to which this package can be re-used.
This PR is one step to unifying the implementation between dskit and Loki.
#281, where loser tree was added to dskit, stated that this was to achieve a 15% performance improvement, but benchmarks on this change give a mixed picture, with some going 5% faster and some 7% slower.
Profiling shows that on my machine 60% of the time goes in initializing random numbers, which could be cached.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]Benchmark results