This repository has been archived by the owner on Jan 13, 2025. It is now read-only.
down samples outgoing gossip pull requests #33719
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
behzadnouri
force-pushed
the
reduce-pull-packets-early
branch
2 times, most recently
from
98bfc27 to
64ae047
Compare
Codecov Report
@@ Coverage Diff @@
## master #33719 +/- ##
=======================================
Coverage 81.8% 81.8%
=======================================
Files 806 806
Lines 217854 217874 +20
=======================================
+ Hits 178207 178256 +49
+ Misses 39647 39618 -29 |
Push message propagation has improved in recent versions of the gossip code and we don't rely on pull requests as much as before. Handling pull requests is also inefficient and expensive. The commit reduces number of outgoing pull requests by down sampling.
behzadnouri
force-pushed
the
reduce-pull-packets-early
branch
from
64ae047 to
490f7f7
Compare
gregcusack
reviewed
Oct 17, 2023
Comment on lines
+155
to
+163
| let mut filters: Vec<_> = repeat_with(|| None).take(1usize << mask_bits).collect(); | ||
| let mut indices: Vec<_> = (0..filters.len()).collect(); | ||
| let size = (filters.len() + SAMPLE_RATE - 1) / SAMPLE_RATE; | ||
| for _ in 0..MAX_NUM_FILTERS.min(size) { | ||
| let k = rng.gen_range(0..indices.len()); | ||
| let k = indices.swap_remove(k); | ||
| let filter = Bloom::random(max_items as usize, FALSE_RATE, max_bits as usize); | ||
| filters[k] = Some(AtomicBloom::<Hash>::from(filter)); | ||
| } |
There was a problem hiding this comment.
are you just sending fewer bloom filters to get populated by your neighbor? now filters vec is populated with only some Bloom filters? essentially replacing this logic in new_pull_request():
let mut filters = self.build_crds_filters(thread_pool, crds, bloom_size);
if filters.len() > MAX_NUM_PULL_REQUESTS {
for i in 0..MAX_NUM_PULL_REQUESTS {
let j = rng.gen_range(i..filters.len());
filters.swap(i, j);
}
filters.truncate(MAX_NUM_PULL_REQUESTS);
}
There was a problem hiding this comment.
yeah, that is basically what the commit is doing.
gregcusack
reviewed
Oct 17, 2023
| @@ -53,8 +53,6 @@ use { | |||
| pub const CRDS_GOSSIP_PULL_CRDS_TIMEOUT_MS: u64 = 15000; | |||
| // Retention period of hashes of received outdated values. | |||
| const FAILED_INSERTS_RETENTION_MS: u64 = 20_000; | |||
| // Maximum number of pull requests to send out each time around. | |||
| const MAX_NUM_PULL_REQUESTS: usize = 1024; | |||
There was a problem hiding this comment.
why do this refactor and filter sampling instead of reducing number of MAX_NUM_PULL_REQUESTS
There was a problem hiding this comment.
The MAX_NUM_PULL_REQUESTS is just an upper bound on the number of outgoing requests regardless of crds table size and it is not proportional. The sampling is proportional to the number of requests going out originally.
gregcusack
approved these changes
Oct 18, 2023
mergify bot
pushed a commit
that referenced
this pull request
Oct 18, 2023
Push message propagation has improved in recent versions of the gossip code and we don't rely on pull requests as much as before. Handling pull requests is also inefficient and expensive. The commit reduces number of outgoing pull requests by down sampling. (cherry picked from commit c699bc9)
mergify bot
added a commit
that referenced
this pull request
Oct 18, 2023
#33752) down samples outgoing gossip pull requests (#33719) Push message propagation has improved in recent versions of the gossip code and we don't rely on pull requests as much as before. Handling pull requests is also inefficient and expensive. The commit reduces number of outgoing pull requests by down sampling. (cherry picked from commit c699bc9) Co-authored-by: behzad nouri <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Push message propagation has improved in recent versions of the gossip code and we don't rely on pull requests as much as before. Handling pull requests is also inefficient and expensive.
Summary of Changes
The commit reduces number of outgoing pull requests by down sampling.