From 5567305a5f372eba0baf831117053432f33aca19 Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Sun, 23 May 2021 16:50:19 +0000 Subject: [PATCH] rolls back min number of bloom items for debug builds (#17420) coverage ci builds are have become flaky presumably because of the overhead added in https://github.com/solana-labs/solana/pull/17236 for very small test clusters. This commit uses a smaller min number of bloom items condition on that if debug assertions are enabled or not. Previous attempt at fixing the flakiness: https://github.com/solana-labs/solana/pull/17408 --- core/src/cluster_info.rs | 15 ++++++++------- core/src/crds_gossip_pull.rs | 24 ++++++++++++++++-------- core/src/crds_gossip_push.rs | 6 +++++- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/core/src/cluster_info.rs b/core/src/cluster_info.rs index 56aa854340..c1fb5b2351 100644 --- a/core/src/cluster_info.rs +++ b/core/src/cluster_info.rs @@ -3046,6 +3046,7 @@ pub fn stake_weight_peers( mod tests { use super::*; use crate::{ + crds_gossip_pull::tests::MIN_NUM_BLOOM_FILTERS, crds_value::{CrdsValue, CrdsValueLabel, Vote as CrdsVote}, duplicate_shred::{self, tests::new_rand_shred, MAX_DUPLICATE_SHREDS}, }; @@ -3800,7 +3801,7 @@ mod tests { cluster_info.set_entrypoint(entrypoint.clone()); let (pings, pulls) = cluster_info.new_pull_requests(&thread_pool, None, &HashMap::new()); assert!(pings.is_empty()); - assert_eq!(pulls.len(), 64); + assert_eq!(pulls.len(), MIN_NUM_BLOOM_FILTERS); for (addr, msg) in pulls { assert_eq!(addr, entrypoint.gossip); match msg { @@ -3830,7 +3831,7 @@ mod tests { ); let (pings, pulls) = cluster_info.new_pull_requests(&thread_pool, None, &HashMap::new()); assert_eq!(pings.len(), 1); - assert_eq!(pulls.len(), 64); + assert_eq!(pulls.len(), MIN_NUM_BLOOM_FILTERS); assert_eq!(*cluster_info.entrypoints.read().unwrap(), vec![entrypoint]); } @@ -4019,7 +4020,7 @@ mod tests { // fresh timestamp). There should only be one pull request to `other_node` let (pings, pulls) = cluster_info.new_pull_requests(&thread_pool, None, &stakes); assert!(pings.is_empty()); - assert_eq!(64, pulls.len()); + assert_eq!(pulls.len(), MIN_NUM_BLOOM_FILTERS); assert!(pulls.into_iter().all(|(addr, _)| addr == other_node.gossip)); // Pull request 2: pretend it's been a while since we've pulled from `entrypoint`. There should @@ -4027,21 +4028,21 @@ mod tests { cluster_info.entrypoints.write().unwrap()[0].wallclock = 0; let (pings, pulls) = cluster_info.new_pull_requests(&thread_pool, None, &stakes); assert!(pings.is_empty()); - assert_eq!(pulls.len(), 64 * 2); + assert_eq!(pulls.len(), 2 * MIN_NUM_BLOOM_FILTERS); assert!(pulls .iter() - .take(64) + .take(MIN_NUM_BLOOM_FILTERS) .all(|(addr, _)| *addr == other_node.gossip)); assert!(pulls .iter() - .skip(64) + .skip(MIN_NUM_BLOOM_FILTERS) .all(|(addr, _)| *addr == entrypoint.gossip)); // Pull request 3: `other_node` is present and `entrypoint` was just pulled from. There should // only be one pull request to `other_node` let (pings, pulls) = cluster_info.new_pull_requests(&thread_pool, None, &stakes); assert!(pings.is_empty()); - assert_eq!(pulls.len(), 64); + assert_eq!(pulls.len(), MIN_NUM_BLOOM_FILTERS); assert!(pulls.into_iter().all(|(addr, _)| addr == other_node.gossip)); } diff --git a/core/src/crds_gossip_pull.rs b/core/src/crds_gossip_pull.rs index 603f0875be..bf5c04f830 100644 --- a/core/src/crds_gossip_pull.rs +++ b/core/src/crds_gossip_pull.rs @@ -468,6 +468,9 @@ impl CrdsGossipPull { bloom_size: usize, ) -> Vec { const PAR_MIN_LENGTH: usize = 512; + #[cfg(debug_assertions)] + const MIN_NUM_BLOOM_ITEMS: usize = 512; + #[cfg(not(debug_assertions))] const MIN_NUM_BLOOM_ITEMS: usize = 65_536; let num_items = crds.len() + self.purged_values.len() + self.failed_inserts.len(); let num_items = MIN_NUM_BLOOM_ITEMS.max(num_items); @@ -646,7 +649,7 @@ impl CrdsGossipPull { } } #[cfg(test)] -mod test { +pub(crate) mod tests { use super::*; use crate::cluster_info::MAX_BLOOM_SIZE; use crate::contact_info::ContactInfo; @@ -662,6 +665,11 @@ mod test { }; use std::{iter::repeat_with, time::Duration}; + #[cfg(debug_assertions)] + pub(crate) const MIN_NUM_BLOOM_FILTERS: usize = 1; + #[cfg(not(debug_assertions))] + pub(crate) const MIN_NUM_BLOOM_FILTERS: usize = 64; + #[test] fn test_hash_as_u64() { let arr: Vec = (0..HASH_BYTES).map(|i| i as u8 + 1).collect(); @@ -916,7 +924,7 @@ mod test { } assert_eq!(num_inserts, 20_000); let filters = crds_gossip_pull.build_crds_filters(&thread_pool, &crds, MAX_BLOOM_SIZE); - assert_eq!(filters.len(), 64); + assert_eq!(filters.len(), MIN_NUM_BLOOM_FILTERS.max(32)); let hash_values: Vec<_> = crds .values() .map(|v| v.value_hash) @@ -1171,7 +1179,7 @@ mod test { CRDS_GOSSIP_PULL_MSG_TIMEOUT_MS, ); assert_eq!(rsp[0].len(), 0); - assert_eq!(filters.len(), 64); + assert_eq!(filters.len(), MIN_NUM_BLOOM_FILTERS); filters.extend({ // Should return new value since caller is new. let now = CRDS_GOSSIP_PULL_MSG_TIMEOUT_MS + 1; @@ -1188,12 +1196,12 @@ mod test { /*output_size_limit=*/ usize::MAX, CRDS_GOSSIP_PULL_MSG_TIMEOUT_MS, ); - assert_eq!(rsp.len(), 128); + assert_eq!(rsp.len(), 2 * MIN_NUM_BLOOM_FILTERS); // There should be only one non-empty response in the 2nd half. // Orders are also preserved. - assert!(rsp.iter().take(64).all(|r| r.is_empty())); - assert_eq!(rsp.iter().filter(|r| r.is_empty()).count(), 127); - assert_eq!(rsp.iter().find(|r| !r.is_empty()).unwrap().len(), 1); + assert!(rsp.iter().take(MIN_NUM_BLOOM_FILTERS).all(|r| r.is_empty())); + assert_eq!(rsp.iter().filter(|r| r.is_empty()).count(), rsp.len() - 1); + assert_eq!(rsp.iter().find(|r| r.len() == 1).unwrap().len(), 1); } #[test] @@ -1338,7 +1346,7 @@ mod test { if rsp.is_empty() { continue; } - assert_eq!(rsp.len(), 64); + assert_eq!(rsp.len(), MIN_NUM_BLOOM_FILTERS); let failed = node .process_pull_response( &mut node_crds, diff --git a/core/src/crds_gossip_push.rs b/core/src/crds_gossip_push.rs index 1c9bf18e25..b6a914df86 100644 --- a/core/src/crds_gossip_push.rs +++ b/core/src/crds_gossip_push.rs @@ -265,6 +265,10 @@ impl CrdsGossipPush { ) { const BLOOM_FALSE_RATE: f64 = 0.1; const BLOOM_MAX_BITS: usize = 1024 * 8 * 4; + #[cfg(debug_assertions)] + const MIN_NUM_BLOOM_ITEMS: usize = 512; + #[cfg(not(debug_assertions))] + const MIN_NUM_BLOOM_ITEMS: usize = CRDS_UNIQUE_PUBKEY_CAPACITY; let mut rng = rand::thread_rng(); let need = Self::compute_need(self.num_active, self.active_set.len(), ratio); let mut new_items = HashMap::new(); @@ -281,7 +285,7 @@ impl CrdsGossipPush { if peers.is_empty() { return; } - let num_bloom_items = CRDS_UNIQUE_PUBKEY_CAPACITY.max(network_size); + let num_bloom_items = MIN_NUM_BLOOM_ITEMS.max(network_size); let shuffle = { let mut seed = [0; 32]; rng.fill(&mut seed[..]);