implements an unbiased weighted shuffle using binary indexed tree #18343
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
behzadnouri
force-pushed
the
weighted-shuffle-fenwick
branch
3 times, most recently
from
f866ecd
to
856bfca
Compare
Codecov Report
@@ Coverage Diff @@
## master #18343 +/- ##
========================================
Coverage 82.5% 82.6%
========================================
Files 436 436
Lines 121801 121903 +102
========================================
+ Hits 100602 100693 +91
- Misses 21199 21210 +11 |
Current implementation of weighted_shuffle: https://github.com/solana-labs/solana/blob/b08f8bd1b/gossip/src/weighted_shuffle.rs#L11-L37 uses a heuristic which results in biased samples. For example, if the weights are [1, 10, 100], then the 3rd index should come first 100 times more often than the 1st index. However, weighted_shuffle is picking the 3rd index 200+ times more often than the 1st index, showing a disproportional bias in favor of higher weights. This commit implements weighted shuffle using binary indexed tree to maintain cumulative sum of weights while sampling. The resulting samples are demonstrably unbiased and precisely proportional to the weights. Additionally the iterator interface allows to skip computations when not all indices are processed. Of the use cases of weighted_shuffle, changing turbine code requires feature-gating to keep the cluster in sync. That is not updated in this commit, but can be done together with future updates to turbine.
behzadnouri
force-pushed
the
weighted-shuffle-fenwick
branch
from
856bfca
to
d912b34
Compare
jbiseda
reviewed
Jul 2, 2021
Comment on lines
304
to
+309
| let since = (now.saturating_sub(req_time).min(3600 * 1000) / 1024) as u32; | ||
| let stake = get_stake(&item.id, stakes); | ||
| let weight = get_weight(max_weight, since, stake); | ||
| (weight, item) | ||
| // Weights are bounded by max_weight defined above. | ||
| // So this type-cast should be safe. | ||
| ((weight * 100.0) as u64, item) |
There was a problem hiding this comment.
Add a function for this calculation to use here and in crds_gossip_push.rs?
There was a problem hiding this comment.
Yeah, I think might be good to refactor push_options and pull_options to share the common parts. But I would rather not to mix that refactoring with this pull-request.
mergify bot
pushed a commit
that referenced
this pull request
Jul 7, 2021
…8343) Current implementation of weighted_shuffle: https://github.com/solana-labs/solana/blob/b08f8bd1b/gossip/src/weighted_shuffle.rs#L11-L37 uses a heuristic which results in biased samples. For example, if the weights are [1, 10, 100], then the 3rd index should come first 100 times more often than the 1st index. However, weighted_shuffle is picking the 3rd index 200+ times more often than the 1st index, showing a disproportional bias in favor of higher weights. This commit implements weighted shuffle using binary indexed tree to maintain cumulative sum of weights while sampling. The resulting samples are demonstrably unbiased and precisely proportional to the weights. Additionally the iterator interface allows to skip computations when not all indices are processed. Of the use cases of weighted_shuffle, changing turbine code requires feature-gating to keep the cluster in sync. That is not updated in this commit, but can be done together with future updates to turbine. (cherry picked from commit dba42c5)
mergify bot
added a commit
that referenced
this pull request
Jul 7, 2021
…8343) (#18485) Current implementation of weighted_shuffle: https://github.com/solana-labs/solana/blob/b08f8bd1b/gossip/src/weighted_shuffle.rs#L11-L37 uses a heuristic which results in biased samples. For example, if the weights are [1, 10, 100], then the 3rd index should come first 100 times more often than the 1st index. However, weighted_shuffle is picking the 3rd index 200+ times more often than the 1st index, showing a disproportional bias in favor of higher weights. This commit implements weighted shuffle using binary indexed tree to maintain cumulative sum of weights while sampling. The resulting samples are demonstrably unbiased and precisely proportional to the weights. Additionally the iterator interface allows to skip computations when not all indices are processed. Of the use cases of weighted_shuffle, changing turbine code requires feature-gating to keep the cluster in sync. That is not updated in this commit, but can be done together with future updates to turbine. (cherry picked from commit dba42c5) Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Closed
behzadnouri
added a commit
to behzadnouri/solana
that referenced
this pull request
Apr 5, 2022
Older weighted_shuffle is based on a heuristic which results in biased samples as shown in: solana-labs#18343 and can be replaced with WeightedShuffle. Also, as described in: solana-labs#13919 weighted_best can be replaced with rand::distributions::WeightedIndex, or WeightdShuffle::first.
behzadnouri
added a commit
to behzadnouri/solana
that referenced
this pull request
Apr 5, 2022
Older weighted_shuffle is based on a heuristic which results in biased samples as shown in: solana-labs#18343 and can be replaced with WeightedShuffle. Also, as described in: solana-labs#13919 weighted_best can be replaced with rand::distributions::WeightedIndex, or WeightdShuffle::first.
mergify bot
pushed a commit
that referenced
this pull request
Apr 6, 2022
Older weighted_shuffle is based on a heuristic which results in biased samples as shown in: #18343 and can be replaced with WeightedShuffle. Also, as described in: #13919 weighted_best can be replaced with rand::distributions::WeightedIndex, or WeightdShuffle::first. (cherry picked from commit db23295)
behzadnouri
added a commit
that referenced
this pull request
Apr 7, 2022
…24139) Older weighted_shuffle is based on a heuristic which results in biased samples as shown in: #18343 and can be replaced with WeightedShuffle. Also, as described in: #13919 weighted_best can be replaced with rand::distributions::WeightedIndex, or WeightdShuffle::first. (cherry picked from commit db23295) Co-authored-by: behzad nouri <behzadnouri@gmail.com>
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.
Problem
Current implementation of weighted_shuffle:
https://github.com/solana-labs/solana/blob/b08f8bd1b/gossip/src/weighted_shuffle.rs#L11-L37
uses a heuristic which results in biased samples.
For example, if the weights are [1, 10, 100], then the 3rd index should
come first 100 times more often than the 1st index. However,
weighted_shuffle is picking the 3rd index 200+ times more often than the
1st index, showing a disproportional bias in favor of higher weights.
Summary of Changes
This commit implements weighted shuffle using binary indexed tree to
maintain cumulative sum of weights while sampling. The resulting samples
are demonstrably unbiased and precisely proportional to the weights.
Additionally the iterator interface allows to skip computations when
not all indices are processed.
Of the use cases of weighted_shuffle, changing turbine code requires
feature-gating to keep the cluster in sync. That is not updated in
this commit, but can be done together with future updates to turbine.