Skip to content

Commit

Permalink
Obsolete uncleaned_roots in accounts index. (#4092)
Browse files Browse the repository at this point in the history 
* remove uncleaned_slots

* add clean test to bank

* pr

* pr: remove uncleaned_roots_us

* pr: fix wrong code comments

* pr

* fix merge conflicts

* pr

---------

Co-authored-by: HaoranYi <[email protected]>
  • Loading branch information
@HaoranYi
HaoranYi and HaoranYi authored Dec 31, 2024
1 parent c539955 commit d64e0a1
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 221 deletions.
67 changes: 12 additions & 55 deletions accounts-db/src/accounts_db.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2198,15 +2198,6 @@ impl AccountsDb {
reclaim_result
}

fn do_reset_uncleaned_roots(&self, max_clean_root: Option<Slot>) {
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
Expand Down Expand Up @@ -2539,8 +2530,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);
}
Expand Down Expand Up @@ -2801,7 +2790,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);
Expand Down Expand Up @@ -2866,22 +2854,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
Expand Down Expand Up @@ -2947,7 +2919,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");
Expand Down Expand Up @@ -3125,14 +3096,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
Expand Down Expand Up @@ -3161,13 +3124,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
Expand Down Expand Up @@ -3881,7 +3837,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
Expand All @@ -3906,7 +3861,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<AccountStorageEntry>) {
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.
Expand All @@ -3923,10 +3878,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::<AliveAccounts<'_>>(store, &unique_accounts, &self.shrink_stats);
self.shrink_collect::<AliveAccounts<'_>>(&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.
Expand All @@ -3937,7 +3892,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 {
Expand Down Expand Up @@ -4110,7 +4065,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)
}
}
}
Expand Down Expand Up @@ -4671,7 +4626,7 @@ impl AccountsDb {
.num_ancient_slots_shrunk
.fetch_add(1, Ordering::Relaxed);
}
self.shrink_storage(&slot_shrink_candidate);
self.shrink_storage(slot_shrink_candidate);
});
})
});
Expand Down Expand Up @@ -6316,7 +6271,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)
}

Expand Down Expand Up @@ -8926,8 +8880,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);
Expand All @@ -8946,7 +8898,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]);
}
}

Expand Down Expand Up @@ -9280,6 +9231,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) {
Expand Down
2 changes: 0 additions & 2 deletions accounts-db/src/accounts_db/stats.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,11 +271,9 @@ 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,
pub uncleaned_roots_slot_list_1: AtomicU64,
pub get_account_sizes_us: AtomicU64,
pub slots_cleaned: AtomicU64,
}
Expand Down
67 changes: 1 addition & 66 deletions accounts-db/src/accounts_db/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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::<Vec<_>>();
uncleaned_roots.sort_unstable();
assert_eq!(uncleaned_roots, slots);
assert_eq!(
accounts_db.accounts_cache.fetch_max_flush_root(),
alive_slot,
Expand Down Expand Up @@ -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::<Vec<_>>();
uncleaned_roots.sort_unstable();
assert_eq!(uncleaned_roots, slots);
assert_eq!(
accounts_db.accounts_cache.fetch_max_flush_root(),
*slots.last().unwrap()
Expand Down Expand Up @@ -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::<Vec<_>>();
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
Expand All @@ -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,
Expand Down Expand Up @@ -4865,7 +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.accounts_index.clone_uncleaned_roots().contains(&1));
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
Expand Down Expand Up @@ -7795,11 +7733,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));
}

Expand Down
Loading

0 comments on commit d64e0a1

Please sign in to comment.