From b91da2242dca483f59bae1b6c701365894a07cd9 Mon Sep 17 00:00:00 2001 From: steviez Date: Fri, 10 Nov 2023 17:27:43 -0600 Subject: [PATCH] Change Blockstore max_root from RwLock to AtomicU64 (#33998) The Blockstore currently maintains a RwLock of the maximum root it has seen inserted. The value is initialized during Blockstore::open() and updated during calls to Blockstore::set_roots(). The max root is queried fairly often for several use cases, and caching the value is cheaper than constructing an iterator to look it up every time. However, the access patterns of these RwLock match that of an atomic. That is, there is no critical section of code that is run while the lock is head. Rather, read/write locks are acquired in order to read/ update, respectively. So, change the RwLock to an AtomicU64. --- core/src/consensus.rs | 14 ++-- core/src/validator.rs | 2 +- gossip/src/duplicate_shred_handler.rs | 4 +- ledger-tool/src/bigtable.rs | 2 +- ledger/src/blockstore.rs | 94 +++++++++++++-------------- 5 files changed, 55 insertions(+), 61 deletions(-) diff --git a/core/src/consensus.rs b/core/src/consensus.rs index 08b72ebf1..72a0c39bc 100644 --- a/core/src/consensus.rs +++ b/core/src/consensus.rs @@ -1452,7 +1452,7 @@ impl ExternalRootSource { pub fn reconcile_blockstore_roots_with_external_source( external_source: ExternalRootSource, blockstore: &Blockstore, - // blockstore.last_root() might have been updated already. + // blockstore.max_root() might have been updated already. // so take a &mut param both to input (and output iff we update root) last_blockstore_root: &mut Slot, ) -> blockstore_db::Result<()> { @@ -1489,7 +1489,7 @@ pub fn reconcile_blockstore_roots_with_external_source( // Update the caller-managed state of last root in blockstore. // Repeated calls of this function should result in a no-op for // the range of `new_roots`. - *last_blockstore_root = blockstore.last_root(); + *last_blockstore_root = blockstore.max_root(); } else { // This indicates we're in bad state; but still don't panic here. // That's because we might have a chance of recovering properly with @@ -2947,7 +2947,7 @@ pub mod test { reconcile_blockstore_roots_with_external_source( ExternalRootSource::Tower(tower.root()), &blockstore, - &mut blockstore.last_root(), + &mut blockstore.max_root(), ) .unwrap(); @@ -2983,7 +2983,7 @@ pub mod test { reconcile_blockstore_roots_with_external_source( ExternalRootSource::Tower(tower.root()), &blockstore, - &mut blockstore.last_root(), + &mut blockstore.max_root(), ) .unwrap(); } @@ -3004,14 +3004,14 @@ pub mod test { let mut tower = Tower::default(); tower.vote_state.root_slot = Some(4); - assert_eq!(blockstore.last_root(), 0); + assert_eq!(blockstore.max_root(), 0); reconcile_blockstore_roots_with_external_source( ExternalRootSource::Tower(tower.root()), &blockstore, - &mut blockstore.last_root(), + &mut blockstore.max_root(), ) .unwrap(); - assert_eq!(blockstore.last_root(), 0); + assert_eq!(blockstore.max_root(), 0); } #[test] diff --git a/core/src/validator.rs b/core/src/validator.rs index 27ee18dee..d73bf58e8 100644 --- a/core/src/validator.rs +++ b/core/src/validator.rs @@ -1747,7 +1747,7 @@ fn load_blockstore( blockstore.shred_timing_point_sender = poh_timing_point_sender; // following boot sequence (esp BankForks) could set root. so stash the original value // of blockstore root away here as soon as possible. - let original_blockstore_root = blockstore.last_root(); + let original_blockstore_root = blockstore.max_root(); let blockstore = Arc::new(blockstore); let blockstore_root_scan = BlockstoreRootScan::new(config, blockstore.clone(), exit.clone()); diff --git a/gossip/src/duplicate_shred_handler.rs b/gossip/src/duplicate_shred_handler.rs index ba95178bc..1410e8262 100644 --- a/gossip/src/duplicate_shred_handler.rs +++ b/gossip/src/duplicate_shred_handler.rs @@ -78,7 +78,7 @@ impl DuplicateShredHandler { } fn cache_root_info(&mut self) { - let last_root = self.blockstore.last_root(); + let last_root = self.blockstore.max_root(); if last_root == self.last_root && !self.cached_staked_nodes.is_empty() { return; } @@ -361,7 +361,7 @@ mod tests { // This proof will be rejected because the slot is too far away in the future. let future_slot = - blockstore.last_root() + duplicate_shred_handler.cached_slots_in_epoch + start_slot; + blockstore.max_root() + duplicate_shred_handler.cached_slots_in_epoch + start_slot; let chunks = create_duplicate_proof( my_keypair.clone(), None, diff --git a/ledger-tool/src/bigtable.rs b/ledger-tool/src/bigtable.rs index 6f0f3e882..c4d5c77f3 100644 --- a/ledger-tool/src/bigtable.rs +++ b/ledger-tool/src/bigtable.rs @@ -63,7 +63,7 @@ async fn upload( None => blockstore.get_first_available_block()?, }; - let ending_slot = ending_slot.unwrap_or_else(|| blockstore.last_root()); + let ending_slot = ending_slot.unwrap_or_else(|| blockstore.max_root()); while starting_slot <= ending_slot { let current_ending_slot = min( diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index 7f596b055..28c463646 100644 --- a/ledger/src/blockstore.rs +++ b/ledger/src/blockstore.rs @@ -73,7 +73,7 @@ use { path::{Path, PathBuf}, rc::Rc, sync::{ - atomic::{AtomicBool, Ordering}, + atomic::{AtomicBool, AtomicU64, Ordering}, Arc, Mutex, RwLock, }, }, @@ -214,7 +214,7 @@ pub struct Blockstore { program_costs_cf: LedgerColumn, bank_hash_cf: LedgerColumn, optimistic_slots_cf: LedgerColumn, - last_root: RwLock, + max_root: AtomicU64, insert_shreds_lock: Mutex<()>, new_shreds_signals: Mutex>>, completed_slots_senders: Mutex>, @@ -324,7 +324,7 @@ impl Blockstore { .next() .map(|(slot, _)| slot) .unwrap_or(0); - let last_root = RwLock::new(max_root); + let max_root = AtomicU64::new(max_root); measure.stop(); info!("{:?} {}", blockstore_path, measure); @@ -356,7 +356,7 @@ impl Blockstore { completed_slots_senders: Mutex::default(), shred_timing_point_sender: None, insert_shreds_lock: Mutex::<()>::default(), - last_root, + max_root, lowest_cleanup_slot: RwLock::::default(), slots_stats: SlotsStats::default(), }; @@ -471,16 +471,6 @@ impl Blockstore { self.orphans_cf.get(slot) } - /// Returns the max root or 0 if it does not exist. - pub fn max_root(&self) -> Slot { - self.db - .iter::(IteratorMode::End) - .expect("Couldn't get rooted iterator for max_root()") - .next() - .map(|(slot, _)| slot) - .unwrap_or(0) - } - pub fn slot_meta_iterator( &self, slot: Slot, @@ -1192,7 +1182,7 @@ impl Blockstore { return false; } - if !Blockstore::should_insert_coding_shred(&shred, &self.last_root) { + if !Blockstore::should_insert_coding_shred(&shred, self.max_root()) { metrics.num_coding_shreds_invalid += 1; return false; } @@ -1391,7 +1381,7 @@ impl Blockstore { &shred, slot_meta, just_inserted_shreds, - &self.last_root, + self.max_root(), leader_schedule, shred_source, duplicate_shreds, @@ -1419,9 +1409,9 @@ impl Blockstore { Ok(newly_completed_data_sets) } - fn should_insert_coding_shred(shred: &Shred, last_root: &RwLock) -> bool { + fn should_insert_coding_shred(shred: &Shred, max_root: Slot) -> bool { debug_assert_matches!(shred.sanitize(), Ok(())); - shred.is_code() && shred.slot() > *last_root.read().unwrap() + shred.is_code() && shred.slot() > max_root } fn insert_coding_shred( @@ -1473,7 +1463,7 @@ impl Blockstore { shred: &Shred, slot_meta: &SlotMeta, just_inserted_shreds: &HashMap, - last_root: &RwLock, + max_root: Slot, leader_schedule: Option<&LeaderScheduleCache>, shred_source: ShredSource, duplicate_shreds: &mut Vec, @@ -1568,12 +1558,11 @@ impl Blockstore { return false; } - let last_root = *last_root.read().unwrap(); // TODO Shouldn't this use shred.parent() instead and update // slot_meta.parent_slot accordingly? slot_meta .parent_slot - .map(|parent_slot| verify_shred_slots(slot, parent_slot, last_root)) + .map(|parent_slot| verify_shred_slots(slot, parent_slot, max_root)) .unwrap_or_default() } @@ -1584,7 +1573,7 @@ impl Blockstore { sender, SlotPohTimingInfo::new_slot_full_poh_time_point( slot, - Some(self.last_root()), + Some(self.max_root()), solana_sdk::timing::timestamp(), ), ); @@ -2491,10 +2480,10 @@ impl Blockstore { "blockstore-rpc-api", ("method", "get_complete_transaction", String) ); - let last_root = self.last_root(); + let max_root = self.max_root(); let confirmed_unrooted_slots: HashSet<_> = AncestorIterator::new_inclusive(highest_confirmed_slot, self) - .take_while(|&slot| slot > last_root) + .take_while(|&slot| slot > max_root) .collect(); self.get_transaction_with_status(signature, &confirmed_unrooted_slots) } @@ -2642,10 +2631,10 @@ impl Blockstore { "blockstore-rpc-api", ("method", "get_confirmed_signatures_for_address2", String) ); - let last_root = self.last_root(); + let max_root = self.max_root(); let confirmed_unrooted_slots: HashSet<_> = AncestorIterator::new_inclusive(highest_slot, self) - .take_while(|&slot| slot > last_root) + .take_while(|&slot| slot > max_root) .collect(); // Figure the `slot` to start listing signatures at, based on the ledger location of the @@ -3247,12 +3236,8 @@ impl Blockstore { } self.db.write(write_batch)?; - - let mut last_root = self.last_root.write().unwrap(); - if *last_root == std::u64::MAX { - *last_root = 0; - } - *last_root = cmp::max(max_new_rooted_slot, *last_root); + self.max_root + .fetch_max(max_new_rooted_slot, Ordering::Relaxed); Ok(()) } @@ -3355,8 +3340,17 @@ impl Blockstore { Ok(duplicate_slots_iterator.map(|(slot, _)| slot)) } + /// Returns the max root or 0 if it does not exist + pub fn max_root(&self) -> Slot { + self.max_root.load(Ordering::Relaxed) + } + + #[deprecated( + since = "1.18.0", + note = "Please use `solana_ledger::blockstore::Blockstore::max_root()` instead" + )] pub fn last_root(&self) -> Slot { - *self.last_root.read().unwrap() + self.max_root() } // find the first available slot in blockstore that has some data in it @@ -3370,7 +3364,7 @@ impl Blockstore { } } // This means blockstore is empty, should never get here aside from right at boot. - self.last_root() + self.max_root() } fn lowest_slot_with_genesis(&self) -> Slot { @@ -3383,7 +3377,7 @@ impl Blockstore { } } // This means blockstore is empty, should never get here aside from right at boot. - self.last_root() + self.max_root() } /// Returns the highest available slot in the blockstore @@ -3458,7 +3452,7 @@ impl Blockstore { } slot } else { - self.last_root() + self.max_root() }; let end_slot = end_slot.unwrap_or(*lowest_cleanup_slot); let ancestor_iterator = @@ -6590,7 +6584,7 @@ pub mod tests { let ledger_path = get_tmp_ledger_path_auto_delete!(); let blockstore = Blockstore::open(ledger_path.path()).unwrap(); - let last_root = RwLock::new(0); + let max_root = 0; // Insert the first 5 shreds, we don't have a "is_last" shred yet blockstore @@ -6620,7 +6614,7 @@ pub mod tests { &empty_shred, &slot_meta, &HashMap::new(), - &last_root, + max_root, None, ShredSource::Repaired, &mut Vec::new(), @@ -6645,7 +6639,7 @@ pub mod tests { &shred7, &slot_meta, &HashMap::new(), - &last_root, + max_root, None, ShredSource::Repaired, &mut duplicate_shreds, @@ -6676,7 +6670,7 @@ pub mod tests { &shred8, &slot_meta, &HashMap::new(), - &last_root, + max_root, None, ShredSource::Repaired, &mut duplicate_shreds, @@ -6784,7 +6778,7 @@ pub mod tests { fn test_should_insert_coding_shred() { let ledger_path = get_tmp_ledger_path_auto_delete!(); let blockstore = Blockstore::open(ledger_path.path()).unwrap(); - let last_root = RwLock::new(0); + let max_root = 0; let slot = 1; let mut coding_shred = Shred::new_from_parity_shard( @@ -6801,7 +6795,7 @@ pub mod tests { // Insert a good coding shred assert!(Blockstore::should_insert_coding_shred( &coding_shred, - &last_root + max_root )); // Insertion should succeed @@ -6814,7 +6808,7 @@ pub mod tests { { assert!(Blockstore::should_insert_coding_shred( &coding_shred, - &last_root + max_root )); } @@ -6822,16 +6816,16 @@ pub mod tests { coding_shred.set_index(coding_shred.index() + 1); assert!(Blockstore::should_insert_coding_shred( &coding_shred, - &last_root + max_root )); // Trying to insert value into slot <= than last root should fail { let mut coding_shred = coding_shred.clone(); - coding_shred.set_slot(*last_root.read().unwrap()); + coding_shred.set_slot(max_root); assert!(!Blockstore::should_insert_coding_shred( &coding_shred, - &last_root + max_root )); } } @@ -6896,11 +6890,11 @@ pub mod tests { let ledger_path = get_tmp_ledger_path_auto_delete!(); let blockstore = Blockstore::open(ledger_path.path()).unwrap(); let chained_slots = vec![0, 2, 4, 7, 12, 15]; - assert_eq!(blockstore.last_root(), 0); + assert_eq!(blockstore.max_root(), 0); blockstore.set_roots(chained_slots.iter()).unwrap(); - assert_eq!(blockstore.last_root(), 15); + assert_eq!(blockstore.max_root(), 15); for i in chained_slots { assert!(blockstore.is_root(i)); @@ -7071,9 +7065,9 @@ pub mod tests { // Make shred for slot 1 let (shreds1, _) = make_slot_entries(1, 0, 1, /*merkle_variant:*/ true); - let last_root = 100; + let max_root = 100; - blockstore.set_roots(std::iter::once(&last_root)).unwrap(); + blockstore.set_roots(std::iter::once(&max_root)).unwrap(); // Insert will fail, slot < root blockstore