From eca22db772529dab2726beb0021d9974b23d1ddb Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Thu, 12 Dec 2024 22:46:07 +0000 Subject: [PATCH 1/8] remove uncleaned_slots --- accounts-db/src/accounts_db.rs | 54 ++------------- accounts-db/src/accounts_db/stats.rs | 1 - accounts-db/src/accounts_db/tests.rs | 86 +----------------------- accounts-db/src/accounts_index.rs | 98 ---------------------------- 4 files changed, 9 insertions(+), 230 deletions(-) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index 03bbdd98387679..0d1b46da8957ec 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -2205,15 +2205,6 @@ impl AccountsDb { reclaim_result } - fn do_reset_uncleaned_roots(&self, max_clean_root: Option) { - let mut measure = Measure::start("reset"); - self.accounts_index.reset_uncleaned_roots(max_clean_root); - measure.stop(); - self.clean_accounts_stats - .reset_uncleaned_roots_us - .fetch_add(measure.as_us(), Ordering::Relaxed); - } - /// increment store_counts to non-zero for all stores that can not be deleted. /// a store cannot be deleted if: /// 1. one of the pubkeys in the store has account info to a store whose store count is not going to zero @@ -2546,8 +2537,6 @@ impl AccountsDb { slot_one_epoch_old.saturating_sub(acceptable_straggler_slot_count); let (old_storages, old_slots) = self.get_storages(..old_slot_cutoff); let num_old_storages = old_storages.len(); - self.accounts_index - .add_uncleaned_roots(old_slots.iter().copied()); for (old_slot, old_storage) in std::iter::zip(old_slots, old_storages) { self.dirty_stores.entry(old_slot).or_insert(old_storage); } @@ -2808,7 +2797,6 @@ impl AccountsDb { let num_candidates = Self::count_pubkeys(&candidates); let mut accounts_scan = Measure::start("accounts_scan"); - let uncleaned_roots = self.accounts_index.clone_uncleaned_roots(); let found_not_zero_accum = AtomicU64::new(0); let not_found_on_fork_accum = AtomicU64::new(0); let missing_accum = AtomicU64::new(0); @@ -2873,22 +2861,6 @@ impl AccountsDb { purges_old_accounts_local += 1; useless = false; } - // Note, this next if-block is only kept to maintain the - // `uncleaned_roots_slot_list_1` stat. - if uncleaned_roots.contains(slot) { - // Assertion enforced by `accounts_index.get()`, the latest slot - // will not be greater than the given `max_clean_root` - if let Some(max_clean_root_inclusive) = - max_clean_root_inclusive - { - assert!(slot <= &max_clean_root_inclusive); - } - if slot_list.len() == 1 { - self.clean_accounts_stats - .uncleaned_roots_slot_list_1 - .fetch_add(1, Ordering::Relaxed); - } - } } None => { // This pubkey is in the index but not in a root slot, so clean @@ -2954,7 +2926,6 @@ impl AccountsDb { let mut clean_old_rooted = Measure::start("clean_old_roots"); let (purged_account_slots, removed_accounts) = self.clean_accounts_older_than_root(&reclaims, &pubkeys_removed_from_accounts_index); - self.do_reset_uncleaned_roots(max_clean_root_inclusive); clean_old_rooted.stop(); let mut store_counts_time = Measure::start("store_counts"); @@ -3132,14 +3103,6 @@ impl AccountsDb { i64 ), ("scan_missing", missing_accum.load(Ordering::Relaxed), i64), - ("uncleaned_roots_len", uncleaned_roots.len(), i64), - ( - "uncleaned_roots_slot_list_1", - self.clean_accounts_stats - .uncleaned_roots_slot_list_1 - .swap(0, Ordering::Relaxed), - i64 - ), ( "get_account_sizes_us", self.clean_accounts_stats @@ -3888,7 +3851,6 @@ impl AccountsDb { if store.num_zero_lamport_single_ref_accounts() == store.count() { // all accounts in this storage can be dead - self.accounts_index.add_uncleaned_roots([slot]); self.dirty_stores.entry(slot).or_insert(store); self.shrink_stats .num_dead_slots_added_to_clean @@ -3913,7 +3875,7 @@ impl AccountsDb { } /// Shrinks `store` by rewriting the alive accounts to a new storage - fn shrink_storage(&self, store: &AccountStorageEntry) { + fn shrink_storage(&self, store: Arc) { let slot = store.slot(); if self.accounts_cache.contains(slot) { // It is not correct to shrink a slot while it is in the write cache until flush is complete and the slot is removed from the write cache. @@ -3930,10 +3892,10 @@ impl AccountsDb { return; } let unique_accounts = - self.get_unique_accounts_from_storage_for_shrink(store, &self.shrink_stats); + self.get_unique_accounts_from_storage_for_shrink(&store, &self.shrink_stats); debug!("do_shrink_slot_store: slot: {}", slot); let shrink_collect = - self.shrink_collect::>(store, &unique_accounts, &self.shrink_stats); + self.shrink_collect::>(&store, &unique_accounts, &self.shrink_stats); // This shouldn't happen if alive_bytes is accurate. // However, it is possible that the remaining alive bytes could be 0. In that case, the whole slot should be marked dead by clean. @@ -3944,7 +3906,7 @@ impl AccountsDb { { if shrink_collect.alive_total_bytes == 0 { // clean needs to take care of this dead slot - self.accounts_index.add_uncleaned_roots([slot]); + self.dirty_stores.insert(slot, store.clone()); } if !shrink_collect.all_are_zero_lamports { @@ -4117,7 +4079,7 @@ impl AccountsDb { .get_slot_storage_entry_shrinking_in_progress_ok(slot) { if Self::is_shrinking_productive(&store) { - self.shrink_storage(&store) + self.shrink_storage(store) } } } @@ -4678,7 +4640,7 @@ impl AccountsDb { .num_ancient_slots_shrunk .fetch_add(1, Ordering::Relaxed); } - self.shrink_storage(&slot_shrink_candidate); + self.shrink_storage(slot_shrink_candidate); }); }) }); @@ -6327,7 +6289,6 @@ impl AccountsDb { // Only add to the uncleaned roots set *after* we've flushed the previous roots, // so that clean will actually be able to clean the slots. let num_new_roots = cached_roots.len(); - self.accounts_index.add_uncleaned_roots(cached_roots); (num_new_roots, num_roots_flushed, flush_stats) } @@ -8936,8 +8897,6 @@ impl AccountsDb { timings.slots_to_clean = uncleaned_roots.len() as u64; timings.num_duplicate_accounts = num_duplicate_accounts; - self.accounts_index - .add_uncleaned_roots(uncleaned_roots.into_iter()); accounts_data_len.fetch_sub(accounts_data_len_from_duplicates, Ordering::Relaxed); if let Some(duplicates_lt_hash) = duplicates_lt_hash { let old_val = outer_duplicates_lt_hash.replace(duplicates_lt_hash); @@ -8956,7 +8915,6 @@ impl AccountsDb { ); for (slot, storage) in all_zero_slots_to_clean { self.dirty_stores.insert(slot, storage); - self.accounts_index.add_uncleaned_roots([slot]); } } diff --git a/accounts-db/src/accounts_db/stats.rs b/accounts-db/src/accounts_db/stats.rs index d33ae9c1f63658..dc81731bd348fd 100644 --- a/accounts-db/src/accounts_db/stats.rs +++ b/accounts-db/src/accounts_db/stats.rs @@ -279,7 +279,6 @@ pub struct CleanAccountsStats { pub remove_dead_accounts_remove_us: AtomicU64, pub remove_dead_accounts_shrink_us: AtomicU64, pub clean_stored_dead_slots_us: AtomicU64, - pub uncleaned_roots_slot_list_1: AtomicU64, pub get_account_sizes_us: AtomicU64, pub slots_cleaned: AtomicU64, } diff --git a/accounts-db/src/accounts_db/tests.rs b/accounts-db/src/accounts_db/tests.rs index 2f810b9598c6a8..2558ad706675bc 100644 --- a/accounts-db/src/accounts_db/tests.rs +++ b/accounts-db/src/accounts_db/tests.rs @@ -1976,43 +1976,6 @@ fn test_clean_max_slot_zero_lamport_account() { assert!(!accounts.accounts_index.contains_with(&pubkey, None, None)); } -#[test] -fn test_uncleaned_roots_with_account() { - solana_logger::setup(); - - let accounts = AccountsDb::new_single_for_tests(); - let pubkey = solana_sdk::pubkey::new_rand(); - let account = AccountSharedData::new(1, 0, AccountSharedData::default().owner()); - //store an account - accounts.store_for_tests(0, &[(&pubkey, &account)]); - assert_eq!(accounts.accounts_index.uncleaned_roots_len(), 0); - - // simulate slots are rooted after while - accounts.add_root_and_flush_write_cache(0); - assert_eq!(accounts.accounts_index.uncleaned_roots_len(), 1); - - //now uncleaned roots are cleaned up - accounts.clean_accounts_for_tests(); - assert_eq!(accounts.accounts_index.uncleaned_roots_len(), 0); -} - -#[test] -fn test_uncleaned_roots_with_no_account() { - solana_logger::setup(); - - let accounts = AccountsDb::new_single_for_tests(); - - assert_eq!(accounts.accounts_index.uncleaned_roots_len(), 0); - - // simulate slots are rooted after while - accounts.add_root_and_flush_write_cache(0); - assert_eq!(accounts.accounts_index.uncleaned_roots_len(), 1); - - //now uncleaned roots are cleaned up - accounts.clean_accounts_for_tests(); - assert_eq!(accounts.accounts_index.uncleaned_roots_len(), 0); -} - fn assert_no_stores(accounts: &AccountsDb, slot: Slot) { let store = accounts.storage.get_slot_storage_entry(slot); assert!(store.is_none()); @@ -4358,13 +4321,6 @@ fn test_accounts_db_cache_clean_dead_slots() { // If no `max_clean_root` is specified, cleaning should purge all flushed slots accounts_db.flush_accounts_cache(true, None); assert_eq!(accounts_db.accounts_cache.num_slots(), 0); - let mut uncleaned_roots = accounts_db - .accounts_index - .clear_uncleaned_roots(None) - .into_iter() - .collect::>(); - uncleaned_roots.sort_unstable(); - assert_eq!(uncleaned_roots, slots); assert_eq!( accounts_db.accounts_cache.fetch_max_flush_root(), alive_slot, @@ -4412,13 +4368,6 @@ fn test_accounts_db_cache_clean() { // If no `max_clean_root` is specified, cleaning should purge all flushed slots accounts_db.flush_accounts_cache(true, None); assert_eq!(accounts_db.accounts_cache.num_slots(), 0); - let mut uncleaned_roots = accounts_db - .accounts_index - .clear_uncleaned_roots(None) - .into_iter() - .collect::>(); - uncleaned_roots.sort_unstable(); - assert_eq!(uncleaned_roots, slots); assert_eq!( accounts_db.accounts_cache.fetch_max_flush_root(), *slots.last().unwrap() @@ -4469,13 +4418,6 @@ fn run_test_accounts_db_cache_clean_max_root( assert_eq!(accounts_db.accounts_cache.num_slots(), 0,); } - let mut uncleaned_roots = accounts_db - .accounts_index - .clear_uncleaned_roots(None) - .into_iter() - .collect::>(); - uncleaned_roots.sort_unstable(); - let expected_max_flushed_root = if !is_cache_at_limit { // Should flush all slots between 0..=requested_flush_root requested_flush_root @@ -4484,10 +4426,6 @@ fn run_test_accounts_db_cache_clean_max_root( num_slots as Slot - 1 }; - assert_eq!( - uncleaned_roots, - slots[0..=expected_max_flushed_root as usize].to_vec() - ); assert_eq!( accounts_db.accounts_cache.fetch_max_flush_root(), expected_max_flushed_root, @@ -4865,7 +4803,6 @@ fn test_shrink_unref_handle_zero_lamport_single_ref_accounts() { // And now, slot 1 should be marked complete dead, which will be added // to uncleaned slots, which handle dropping dead storage. And it WON'T // be participating shrinking in the next round. - assert!(db.accounts_index.clone_uncleaned_roots().contains(&1)); assert!(!db.shrink_candidate_slots.lock().unwrap().contains(&1)); // Now, make slot 0 dead by updating the remaining key @@ -7795,11 +7732,8 @@ fn test_handle_dropped_roots_for_ancient() { let slot0 = 0; let dropped_roots = vec![slot0]; db.accounts_index.add_root(slot0); - db.accounts_index.add_uncleaned_roots([slot0]); - assert!(db.accounts_index.is_uncleaned_root(slot0)); assert!(db.accounts_index.is_alive_root(slot0)); db.handle_dropped_roots_for_ancient(dropped_roots.into_iter()); - assert!(!db.accounts_index.is_uncleaned_root(slot0)); assert!(!db.accounts_index.is_alive_root(slot0)); } @@ -8168,19 +8102,8 @@ fn test_clean_old_storages_with_reclaims_rooted() { &[(&pubkey, &account), (&Pubkey::new_unique(), &account)], ); accounts_db.add_root_and_flush_write_cache(slot); - // ensure this slot is *not* in the dirty_stores or uncleaned_pubkeys, because we want to - // test cleaning *old* storages, i.e. when they aren't explicitly marked for cleaning - assert!(!accounts_db.dirty_stores.contains_key(&slot)); - assert!(!accounts_db.uncleaned_pubkeys.contains_key(&slot)); } - // add `old_slot` to the dirty stores list to mimic it being picked up as old - let old_storage = accounts_db - .storage - .get_slot_storage_entry_shrinking_in_progress_ok(old_slot) - .unwrap(); - accounts_db.dirty_stores.insert(old_slot, old_storage); - // ensure the slot list for `pubkey` has both the old and new slots let slot_list = accounts_db .accounts_index @@ -8227,14 +8150,11 @@ fn test_clean_old_storages_with_reclaims_unrooted() { &[(&pubkey, &account), (&Pubkey::new_unique(), &account)], ); accounts_db.calculate_accounts_delta_hash(slot); - // ensure this slot is in uncleaned_pubkeys (but not dirty_stores) so it'll be cleaned - assert!(!accounts_db.dirty_stores.contains_key(&slot)); - assert!(accounts_db.uncleaned_pubkeys.contains_key(&slot)); } - - // only `old_slot` should be rooted, not `new_slot` + // do not root `new_slot`! accounts_db.add_root_and_flush_write_cache(old_slot); - assert!(accounts_db.accounts_index.is_alive_root(old_slot)); + + // for this test, `new_slot` must not be a root assert!(!accounts_db.accounts_index.is_alive_root(new_slot)); // ensure the slot list for `pubkey` has both the old and new slots diff --git a/accounts-db/src/accounts_index.rs b/accounts-db/src/accounts_index.rs index 2aae2d80a21553..dd6a8a9c47de1d 100644 --- a/accounts-db/src/accounts_index.rs +++ b/accounts-db/src/accounts_index.rs @@ -18,7 +18,6 @@ use { ThreadPool, }, solana_measure::measure::Measure, - solana_nohash_hasher::IntSet, solana_sdk::{ account::ReadableAccount, clock::{BankId, Slot}, @@ -441,7 +440,6 @@ pub struct RootsTracker { /// Updated every time we add a new root or clean/shrink an append vec into irrelevancy. /// Range is approximately the last N slots where N is # slots per epoch. pub alive_roots: RollingBitField, - uncleaned_roots: IntSet, } impl Default for RootsTracker { @@ -457,7 +455,6 @@ impl RootsTracker { pub fn new(max_width: u64) -> Self { Self { alive_roots: RollingBitField::new(max_width), - uncleaned_roots: IntSet::default(), } } @@ -2013,24 +2010,6 @@ impl + Into> AccountsIndex { w_roots_tracker.alive_roots.insert(slot); } - pub fn add_uncleaned_roots(&self, roots: I) - where - I: IntoIterator, - { - let mut w_roots_tracker = self.roots_tracker.write().unwrap(); - w_roots_tracker.uncleaned_roots.extend(roots); - } - - /// Removes `root` from `uncleaned_roots` and returns whether it was previously present - #[cfg(feature = "dev-context-only-utils")] - pub fn remove_uncleaned_root(&self, root: Slot) -> bool { - self.roots_tracker - .write() - .unwrap() - .uncleaned_roots - .remove(&root) - } - pub fn max_root_inclusive(&self) -> Slot { self.roots_tracker .read() @@ -2044,12 +2023,7 @@ impl + Into> AccountsIndex { /// return true if slot was a root pub fn clean_dead_slot(&self, slot: Slot) -> bool { let mut w_roots_tracker = self.roots_tracker.write().unwrap(); - let removed_from_unclean_roots = w_roots_tracker.uncleaned_roots.remove(&slot); if !w_roots_tracker.alive_roots.remove(&slot) { - if removed_from_unclean_roots { - error!("clean_dead_slot-removed_from_unclean_roots: {}", slot); - inc_new_counter_error!("clean_dead_slot-removed_from_unclean_roots", 1, 1); - } false } else { drop(w_roots_tracker); @@ -2061,7 +2035,6 @@ impl + Into> AccountsIndex { pub(crate) fn update_roots_stats(&self, stats: &mut AccountsIndexRootsStats) { let roots_tracker = self.roots_tracker.read().unwrap(); stats.roots_len = Some(roots_tracker.alive_roots.len()); - stats.uncleaned_roots_len = Some(roots_tracker.uncleaned_roots.len()); stats.roots_range = Some(roots_tracker.alive_roots.range_width()); } @@ -2069,17 +2042,6 @@ impl + Into> AccountsIndex { self.roots_tracker.read().unwrap().min_alive_root() } - pub(crate) fn reset_uncleaned_roots(&self, max_clean_root: Option) { - let mut w_roots_tracker = self.roots_tracker.write().unwrap(); - w_roots_tracker.uncleaned_roots.retain(|root| { - let is_cleaned = max_clean_root - .map(|max_clean_root| *root <= max_clean_root) - .unwrap_or(true); - // Only keep the slots that have yet to be cleaned - !is_cleaned - }); - } - pub fn num_alive_roots(&self) -> usize { self.roots_tracker.read().unwrap().alive_roots.len() } @@ -2089,14 +2051,6 @@ impl + Into> AccountsIndex { tracker.alive_roots.get_all() } - pub fn clone_uncleaned_roots(&self) -> IntSet { - self.roots_tracker.read().unwrap().uncleaned_roots.clone() - } - - pub fn uncleaned_roots_len(&self) -> usize { - self.roots_tracker.read().unwrap().uncleaned_roots.len() - } - // These functions/fields are only usable from a dev context (i.e. tests and benches) #[cfg(feature = "dev-context-only-utils")] // filter any rooted entries and return them along with a bool that indicates @@ -3141,34 +3095,6 @@ pub mod tests { assert!(index.is_alive_root(0)); } - #[test] - fn test_clean_and_unclean_slot() { - let index = AccountsIndex::::default_for_tests(); - assert_eq!(0, index.roots_tracker.read().unwrap().uncleaned_roots.len()); - index.add_root(0); - index.add_root(1); - index.add_uncleaned_roots([0, 1]); - assert_eq!(2, index.roots_tracker.read().unwrap().uncleaned_roots.len()); - - index.reset_uncleaned_roots(None); - assert_eq!(2, index.roots_tracker.read().unwrap().alive_roots.len()); - assert_eq!(0, index.roots_tracker.read().unwrap().uncleaned_roots.len()); - - index.add_root(2); - index.add_root(3); - index.add_uncleaned_roots([2, 3]); - assert_eq!(4, index.roots_tracker.read().unwrap().alive_roots.len()); - assert_eq!(2, index.roots_tracker.read().unwrap().uncleaned_roots.len()); - - index.clean_dead_slot(1); - assert_eq!(3, index.roots_tracker.read().unwrap().alive_roots.len()); - assert_eq!(2, index.roots_tracker.read().unwrap().uncleaned_roots.len()); - - index.clean_dead_slot(2); - assert_eq!(2, index.roots_tracker.read().unwrap().alive_roots.len()); - assert_eq!(1, index.roots_tracker.read().unwrap().uncleaned_roots.len()); - } - #[test] fn test_update_last_wins() { let key = solana_sdk::pubkey::new_rand(); @@ -4065,30 +3991,6 @@ pub mod tests { assert!(gc.is_empty()); } - pub fn clear_uncleaned_roots(&self, max_clean_root: Option) -> HashSet { - let mut cleaned_roots = HashSet::new(); - let mut w_roots_tracker = self.roots_tracker.write().unwrap(); - w_roots_tracker.uncleaned_roots.retain(|root| { - let is_cleaned = max_clean_root - .map(|max_clean_root| *root <= max_clean_root) - .unwrap_or(true); - if is_cleaned { - cleaned_roots.insert(*root); - } - // Only keep the slots that have yet to be cleaned - !is_cleaned - }); - cleaned_roots - } - - pub(crate) fn is_uncleaned_root(&self, slot: Slot) -> bool { - self.roots_tracker - .read() - .unwrap() - .uncleaned_roots - .contains(&slot) - } - pub fn clear_roots(&self) { self.roots_tracker.write().unwrap().alive_roots.clear() } From 256c4de6c8cb8dd8c9037d85378a8270736a2457 Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Fri, 13 Dec 2024 21:17:47 +0000 Subject: [PATCH 2/8] add clean test to bank --- accounts-db/src/accounts_db.rs | 5 +++ runtime/src/bank/tests.rs | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index 0d1b46da8957ec..f29e31cdb8922f 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -9177,6 +9177,11 @@ impl AccountsDb { ); } } + + #[cfg(feature = "dev-context-only-utils")] + pub fn get_len_of_slots_with_uncleaned_pubkeys(&self) -> usize { + self.uncleaned_pubkeys.len() + } } /// Specify the source of the accounts data when calculating the accounts hash diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 1cb8c7ab4b5a22..cc921aee0eedec 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -13525,3 +13525,60 @@ fn test_rehash_bad() { // let the show begin bank.rehash(); } + +/// Test that when a bank freezes, it populate `uncleaned_pubkeys` for the slot +/// to clean. And after clean, it will remove the slot from `uncleaned_pubkeys`. +#[test] +fn test_populate_uncleaned_slot_for_bank() { + let ten_sol = 10 * LAMPORTS_PER_SOL; + let bank = create_simple_test_bank(100); + let account = AccountSharedData::new(ten_sol, 0, &Pubkey::default()); + let pubkey = Pubkey::new_unique(); + bank.store_account_and_update_capitalization(&pubkey, &account); + bank.freeze(); + + assert_eq!( + bank.rc + .accounts + .accounts_db + .get_len_of_slots_with_uncleaned_pubkeys(), + 1 + ); + + bank.clean_accounts(); + assert_eq!( + bank.rc + .accounts + .accounts_db + .get_len_of_slots_with_uncleaned_pubkeys(), + 0 + ); +} + +/// This test is similar to `test_populate_uncleaned_slot_for_bank`, but it +/// tests when there is no accounts in the bank. In theory, we should optimize +/// and not populate the slot to clean. But the current code didn't check if +/// there are accounts in the bank, so it will populate the slot to clean even +/// it is empty. This test is to make sure the behavior is consistent. +#[test] +fn test_populate_uncleaned_slot_for_bank_with_empty_accounts() { + let bank = create_simple_test_bank(100); + bank.freeze(); + + assert_eq!( + bank.rc + .accounts + .accounts_db + .get_len_of_slots_with_uncleaned_pubkeys(), + 1 + ); + + bank.clean_accounts(); + assert_eq!( + bank.rc + .accounts + .accounts_db + .get_len_of_slots_with_uncleaned_pubkeys(), + 0 + ); +} From c1a33c1e210ff4ed4f44e97a614541d565fc836e Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Fri, 13 Dec 2024 22:59:58 +0000 Subject: [PATCH 3/8] pr --- accounts-db/src/accounts_db.rs | 11 ++++++----- accounts-db/src/accounts_db/tests.rs | 7 +++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index f29e31cdb8922f..9a0916345b1734 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -9177,11 +9177,6 @@ impl AccountsDb { ); } } - - #[cfg(feature = "dev-context-only-utils")] - pub fn get_len_of_slots_with_uncleaned_pubkeys(&self) -> usize { - self.uncleaned_pubkeys.len() - } } /// Specify the source of the accounts data when calculating the accounts hash @@ -9253,6 +9248,12 @@ impl AccountStorageEntry { // These functions/fields are only usable from a dev context (i.e. tests and benches) #[cfg(feature = "dev-context-only-utils")] impl AccountsDb { + /// Return the number of slots marked with uncleaned pubkeys. + /// This is useful for testing clean aglorithms. + pub fn get_len_of_slots_with_uncleaned_pubkeys(&self) -> usize { + self.uncleaned_pubkeys.len() + } + /// useful to adapt tests written prior to introduction of the write cache /// to use the write cache pub fn add_root_and_flush_write_cache(&self, slot: Slot) { diff --git a/accounts-db/src/accounts_db/tests.rs b/accounts-db/src/accounts_db/tests.rs index 2558ad706675bc..77f8d9a1847e4e 100644 --- a/accounts-db/src/accounts_db/tests.rs +++ b/accounts-db/src/accounts_db/tests.rs @@ -4803,6 +4803,7 @@ fn test_shrink_unref_handle_zero_lamport_single_ref_accounts() { // And now, slot 1 should be marked complete dead, which will be added // to uncleaned slots, which handle dropping dead storage. And it WON'T // be participating shrinking in the next round. + assert!(db.dirty_stores.contains_key(&1)); assert!(!db.shrink_candidate_slots.lock().unwrap().contains(&1)); // Now, make slot 0 dead by updating the remaining key @@ -8103,6 +8104,8 @@ fn test_clean_old_storages_with_reclaims_rooted() { ); accounts_db.add_root_and_flush_write_cache(slot); } + // for this test, `new_slot` must be in `dirty_stores`. + assert!(!accounts_db.dirty_stores.contains_key(&new_slot)); // ensure the slot list for `pubkey` has both the old and new slots let slot_list = accounts_db @@ -8154,6 +8157,10 @@ fn test_clean_old_storages_with_reclaims_unrooted() { // do not root `new_slot`! accounts_db.add_root_and_flush_write_cache(old_slot); + // Both slots should be in `uncleaned_pubkeys` after delta hash calculation + assert!(accounts_db.uncleaned_pubkeys.contains_key(&old_slot)); + assert!(accounts_db.uncleaned_pubkeys.contains_key(&new_slot)); + // for this test, `new_slot` must not be a root assert!(!accounts_db.accounts_index.is_alive_root(new_slot)); From 98e2ef31393f02f4dd7878d1737fe39e4062045e Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Mon, 16 Dec 2024 16:15:49 +0000 Subject: [PATCH 4/8] pr: remove uncleaned_roots_us --- accounts-db/src/accounts_db.rs | 7 ------- accounts-db/src/accounts_db/stats.rs | 1 - 2 files changed, 8 deletions(-) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index 9a0916345b1734..0a5fbba2df6666 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -3131,13 +3131,6 @@ impl AccountsDb { .swap(0, Ordering::Relaxed), i64 ), - ( - "reset_uncleaned_roots_us", - self.clean_accounts_stats - .reset_uncleaned_roots_us - .swap(0, Ordering::Relaxed), - i64 - ), ( "remove_dead_accounts_remove_us", self.clean_accounts_stats diff --git a/accounts-db/src/accounts_db/stats.rs b/accounts-db/src/accounts_db/stats.rs index dc81731bd348fd..3af22b907224dc 100644 --- a/accounts-db/src/accounts_db/stats.rs +++ b/accounts-db/src/accounts_db/stats.rs @@ -275,7 +275,6 @@ pub struct CleanAccountsStats { // stats held here and reported by clean_accounts pub clean_old_root_us: AtomicU64, pub clean_old_root_reclaim_us: AtomicU64, - pub reset_uncleaned_roots_us: AtomicU64, pub remove_dead_accounts_remove_us: AtomicU64, pub remove_dead_accounts_shrink_us: AtomicU64, pub clean_stored_dead_slots_us: AtomicU64, From ebcee80190007c858b4ee8dbf4f42d5df71ab7c6 Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Mon, 16 Dec 2024 16:18:15 +0000 Subject: [PATCH 5/8] pr: fix wrong code comments --- accounts-db/src/accounts_db/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accounts-db/src/accounts_db/tests.rs b/accounts-db/src/accounts_db/tests.rs index 77f8d9a1847e4e..387f56f85d74a1 100644 --- a/accounts-db/src/accounts_db/tests.rs +++ b/accounts-db/src/accounts_db/tests.rs @@ -8104,7 +8104,7 @@ fn test_clean_old_storages_with_reclaims_rooted() { ); accounts_db.add_root_and_flush_write_cache(slot); } - // for this test, `new_slot` must be in `dirty_stores`. + // for this test, `new_slot` must not be in `dirty_stores`. assert!(!accounts_db.dirty_stores.contains_key(&new_slot)); // ensure the slot list for `pubkey` has both the old and new slots From 0eb57e4a68d4de68058b61abd4d768c250d77a05 Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Mon, 16 Dec 2024 16:25:29 +0000 Subject: [PATCH 6/8] pr --- accounts-db/src/accounts_db/tests.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/accounts-db/src/accounts_db/tests.rs b/accounts-db/src/accounts_db/tests.rs index 387f56f85d74a1..c29014c2869e4e 100644 --- a/accounts-db/src/accounts_db/tests.rs +++ b/accounts-db/src/accounts_db/tests.rs @@ -8104,7 +8104,14 @@ fn test_clean_old_storages_with_reclaims_rooted() { ); accounts_db.add_root_and_flush_write_cache(slot); } - // for this test, `new_slot` must not be in `dirty_stores`. + // For this test, `new_slot` must **not** be in `dirty_stores`. There are + // two ways to drive `clean`: 1) by "uncleaned" pubkeys, or 2) by "dirty" + // stores. This test is to simulate the case of normal bank rooting process + // to kick off clean. In normal bank rooting process, the "clean" is driven + // by computing the delta hash of the rooted slot. In computing delta hash, + // we directly pick up the pubkeys in the store and set them on + // `cleaned_pubkeys` for clean. We are *not* setting `dirty_store` for this + // case. assert!(!accounts_db.dirty_stores.contains_key(&new_slot)); // ensure the slot list for `pubkey` has both the old and new slots From da7de1930a687e6d35892450349790097eb4f6fb Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Mon, 16 Dec 2024 20:41:36 +0000 Subject: [PATCH 7/8] fix merge conflicts --- accounts-db/src/accounts_db/tests.rs | 31 ++++++++++++++-------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/accounts-db/src/accounts_db/tests.rs b/accounts-db/src/accounts_db/tests.rs index c29014c2869e4e..ea9103547abd12 100644 --- a/accounts-db/src/accounts_db/tests.rs +++ b/accounts-db/src/accounts_db/tests.rs @@ -8103,16 +8103,17 @@ fn test_clean_old_storages_with_reclaims_rooted() { &[(&pubkey, &account), (&Pubkey::new_unique(), &account)], ); accounts_db.add_root_and_flush_write_cache(slot); + // ensure this slot is *not* in the dirty_stores or uncleaned_pubkeys, because we want to + // test cleaning *old* storages, i.e. when they aren't explicitly marked for cleaning + assert!(!accounts_db.dirty_stores.contains_key(&slot)); + assert!(!accounts_db.uncleaned_pubkeys.contains_key(&slot)); } - // For this test, `new_slot` must **not** be in `dirty_stores`. There are - // two ways to drive `clean`: 1) by "uncleaned" pubkeys, or 2) by "dirty" - // stores. This test is to simulate the case of normal bank rooting process - // to kick off clean. In normal bank rooting process, the "clean" is driven - // by computing the delta hash of the rooted slot. In computing delta hash, - // we directly pick up the pubkeys in the store and set them on - // `cleaned_pubkeys` for clean. We are *not* setting `dirty_store` for this - // case. - assert!(!accounts_db.dirty_stores.contains_key(&new_slot)); + // add `old_slot` to the dirty stores list to mimic it being picked up as old + let old_storage = accounts_db + .storage + .get_slot_storage_entry_shrinking_in_progress_ok(old_slot) + .unwrap(); + accounts_db.dirty_stores.insert(old_slot, old_storage); // ensure the slot list for `pubkey` has both the old and new slots let slot_list = accounts_db @@ -8160,15 +8161,13 @@ fn test_clean_old_storages_with_reclaims_unrooted() { &[(&pubkey, &account), (&Pubkey::new_unique(), &account)], ); accounts_db.calculate_accounts_delta_hash(slot); + // ensure this slot is in uncleaned_pubkeys (but not dirty_stores) so it'll be cleaned + assert!(!accounts_db.dirty_stores.contains_key(&slot)); + assert!(accounts_db.uncleaned_pubkeys.contains_key(&slot)); } - // do not root `new_slot`! + // only `old_slot` should be rooted, not `new_slot` accounts_db.add_root_and_flush_write_cache(old_slot); - - // Both slots should be in `uncleaned_pubkeys` after delta hash calculation - assert!(accounts_db.uncleaned_pubkeys.contains_key(&old_slot)); - assert!(accounts_db.uncleaned_pubkeys.contains_key(&new_slot)); - - // for this test, `new_slot` must not be a root + assert!(accounts_db.accounts_index.is_alive_root(old_slot)); assert!(!accounts_db.accounts_index.is_alive_root(new_slot)); // ensure the slot list for `pubkey` has both the old and new slots From 39ca9729484d73dc951e797ecea9751e23fa3257 Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Mon, 16 Dec 2024 21:00:37 +0000 Subject: [PATCH 8/8] pr --- accounts-db/src/accounts_db/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/accounts-db/src/accounts_db/tests.rs b/accounts-db/src/accounts_db/tests.rs index ea9103547abd12..98ff071dc5b45e 100644 --- a/accounts-db/src/accounts_db/tests.rs +++ b/accounts-db/src/accounts_db/tests.rs @@ -8108,6 +8108,7 @@ fn test_clean_old_storages_with_reclaims_rooted() { assert!(!accounts_db.dirty_stores.contains_key(&slot)); assert!(!accounts_db.uncleaned_pubkeys.contains_key(&slot)); } + // add `old_slot` to the dirty stores list to mimic it being picked up as old let old_storage = accounts_db .storage @@ -8165,6 +8166,7 @@ fn test_clean_old_storages_with_reclaims_unrooted() { assert!(!accounts_db.dirty_stores.contains_key(&slot)); assert!(accounts_db.uncleaned_pubkeys.contains_key(&slot)); } + // only `old_slot` should be rooted, not `new_slot` accounts_db.add_root_and_flush_write_cache(old_slot); assert!(accounts_db.accounts_index.is_alive_root(old_slot));