From 2758588ddd74dc0067ee808e14dc9f175de21ba9 Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Tue, 9 Feb 2021 22:04:41 +0000 Subject: [PATCH] uses btree-map instead of hash-map for cluster-slots (#15194) retain traverses all values in the hashmap which is slow: https://github.com/solana-labs/solana/blob/88f22c360/core/src/cluster_slots.rs#L45 btree-map instead allows more efficient prunning there. In addition there is potential race condition here: https://github.com/solana-labs/solana/blob/88f22c360/core/src/cluster_slots.rs#L68-L74 If another thread inserts a value at the same slot key between the read and write lock, current thread will discard the inserted value. --- core/src/cluster_slots.rs | 56 +++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/core/src/cluster_slots.rs b/core/src/cluster_slots.rs index 5f4d455d3a..f016169586 100644 --- a/core/src/cluster_slots.rs +++ b/core/src/cluster_slots.rs @@ -5,16 +5,15 @@ use crate::{ use solana_runtime::{bank_forks::BankForks, epoch_stakes::NodeIdToVoteAccounts}; use solana_sdk::{clock::Slot, pubkey::Pubkey}; use std::{ - collections::{HashMap, HashSet}, + collections::{BTreeMap, HashMap, HashSet}, sync::{Arc, RwLock}, }; pub type SlotPubkeys = HashMap; -pub type ClusterSlotsMap = RwLock>>>; #[derive(Default)] pub struct ClusterSlots { - cluster_slots: ClusterSlotsMap, + cluster_slots: RwLock>>>, since: RwLock>, validator_stakes: RwLock>, epoch: RwLock>, @@ -28,11 +27,10 @@ impl ClusterSlots { pub fn update(&self, root: Slot, cluster_info: &ClusterInfo, bank_forks: &RwLock) { self.update_peers(cluster_info, bank_forks); let since = *self.since.read().unwrap(); - let epoch_slots = cluster_info.get_epoch_slots_since(since); - self.update_internal(root, epoch_slots); + let (epoch_slots, since) = cluster_info.get_epoch_slots_since(since); + self.update_internal(root, epoch_slots, since); } - fn update_internal(&self, root: Slot, epoch_slots: (Vec, Option)) { - let (epoch_slots_list, since) = epoch_slots; + fn update_internal(&self, root: Slot, epoch_slots_list: Vec, since: Option) { for epoch_slots in epoch_slots_list { let slots = epoch_slots.to_slots(root); for slot in &slots { @@ -42,7 +40,10 @@ impl ClusterSlots { self.insert_node_id(*slot, epoch_slots.from); } } - self.cluster_slots.write().unwrap().retain(|x, _| *x > root); + { + let mut cluster_slots = self.cluster_slots.write().unwrap(); + *cluster_slots = cluster_slots.split_off(&(root + 1)); + } *self.since.write().unwrap() = since; } @@ -51,9 +52,8 @@ impl ClusterSlots { .read() .unwrap() .iter() - .filter(|(_, keys)| keys.read().unwrap().get(id).is_some()) - .map(|(slot, _)| slot) - .cloned() + .filter(|(_, keys)| keys.read().unwrap().contains_key(id)) + .map(|(slot, _)| *slot) .collect() } @@ -65,20 +65,14 @@ impl ClusterSlots { .get(&node_id) .map(|v| v.total_stake) .unwrap_or(0); - let mut slot_pubkeys = self.cluster_slots.read().unwrap().get(&slot).cloned(); - if slot_pubkeys.is_none() { - let new_slot_pubkeys = Arc::new(RwLock::new(HashMap::default())); - self.cluster_slots - .write() - .unwrap() - .insert(slot, new_slot_pubkeys.clone()); - slot_pubkeys = Some(new_slot_pubkeys); - } - slot_pubkeys - .unwrap() + let slot_pubkeys = self + .cluster_slots .write() .unwrap() - .insert(node_id, balance); + .entry(slot) + .or_default() + .clone(); + slot_pubkeys.write().unwrap().insert(node_id, balance); } fn update_peers(&self, cluster_info: &ClusterInfo, bank_forks: &RwLock) { @@ -179,7 +173,7 @@ mod tests { #[test] fn test_update_noop() { let cs = ClusterSlots::default(); - cs.update_internal(0, (vec![], None)); + cs.update_internal(0, vec![], None); assert!(cs.cluster_slots.read().unwrap().is_empty()); assert!(cs.since.read().unwrap().is_none()); } @@ -188,7 +182,7 @@ mod tests { fn test_update_empty() { let cs = ClusterSlots::default(); let epoch_slot = EpochSlots::default(); - cs.update_internal(0, (vec![epoch_slot], Some(0))); + cs.update_internal(0, vec![epoch_slot], Some(0)); assert_eq!(*cs.since.read().unwrap(), Some(0)); assert!(cs.lookup(0).is_none()); } @@ -199,7 +193,7 @@ mod tests { let cs = ClusterSlots::default(); let mut epoch_slot = EpochSlots::default(); epoch_slot.fill(&[0], 0); - cs.update_internal(0, (vec![epoch_slot], Some(0))); + cs.update_internal(0, vec![epoch_slot], Some(0)); assert_eq!(*cs.since.read().unwrap(), Some(0)); assert!(cs.lookup(0).is_none()); } @@ -209,7 +203,7 @@ mod tests { let cs = ClusterSlots::default(); let mut epoch_slot = EpochSlots::default(); epoch_slot.fill(&[1], 0); - cs.update_internal(0, (vec![epoch_slot], Some(0))); + cs.update_internal(0, vec![epoch_slot], Some(0)); assert_eq!(*cs.since.read().unwrap(), Some(0)); assert!(cs.lookup(0).is_none()); assert!(cs.lookup(1).is_some()); @@ -340,7 +334,7 @@ mod tests { ); *cs.validator_stakes.write().unwrap() = map; - cs.update_internal(0, (vec![epoch_slot], None)); + cs.update_internal(0, vec![epoch_slot], None); assert!(cs.lookup(1).is_some()); assert_eq!( cs.lookup(1) @@ -357,7 +351,7 @@ mod tests { let cs = ClusterSlots::default(); let mut epoch_slot = EpochSlots::default(); epoch_slot.fill(&[1], 0); - cs.update_internal(0, (vec![epoch_slot], None)); + cs.update_internal(0, vec![epoch_slot], None); let self_id = solana_sdk::pubkey::new_rand(); assert_eq!( cs.generate_repairs_for_missing_slots(&self_id, 0), @@ -371,7 +365,7 @@ mod tests { let mut epoch_slot = EpochSlots::default(); epoch_slot.fill(&[1], 0); let self_id = epoch_slot.from; - cs.update_internal(0, (vec![epoch_slot], None)); + cs.update_internal(0, vec![epoch_slot], None); let slots: Vec = cs.collect(&self_id).into_iter().collect(); assert_eq!(slots, vec![1]); } @@ -382,7 +376,7 @@ mod tests { let mut epoch_slot = EpochSlots::default(); epoch_slot.fill(&[1], 0); let self_id = epoch_slot.from; - cs.update_internal(0, (vec![epoch_slot], None)); + cs.update_internal(0, vec![epoch_slot], None); assert!(cs .generate_repairs_for_missing_slots(&self_id, 0) .is_empty());