Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Commit

Permalink
Keep track of dirty stores on remove accounts to clean
Browse files Browse the repository at this point in the history 
and not zero_lamport key set
  • Loading branch information
@sakridge
sakridge committed Jun 2, 2021
1 parent bbcdf07 commit ef98258
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 39 deletions.
109 changes: 91 additions & 18 deletions runtime/src/accounts_db.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,8 @@ pub struct AccountsDb {
/// by `remove_unrooted_slot()`. Used to ensure `remove_unrooted_slots(slots)`
/// can safely clear the set of unrooted slots `slots`.
remove_unrooted_slots_synchronization: RemoveUnrootedSlotsSynchronization,

dirty_stores: DashMap<(Slot, AppendVecId), Arc<AccountStorageEntry>>,
}

#[derive(Debug, Default)]
Expand Down Expand Up @@ -1290,6 +1292,7 @@ impl Default for AccountsDb {
load_limit: AtomicU64::default(),
is_bank_drop_callback_enabled: AtomicBool::default(),
remove_unrooted_slots_synchronization: RemoveUnrootedSlotsSynchronization::default(),
dirty_stores: DashMap::default(),
}
}
}
Expand Down Expand Up @@ -1609,20 +1612,44 @@ impl AccountsDb {

// Construct a vec of pubkeys for cleaning from:
// uncleaned_pubkeys - the delta set of updated pubkeys in rooted slots from the last clean
// zero_lamport_pubkeys - set of all alive pubkeys containing 0-lamport updates
// dirty_stores - set of stores which had accounts removed
fn construct_candidate_clean_keys(
&self,
max_clean_root: Option<Slot>,
timings: &mut CleanKeyTimings,
) -> Vec<Pubkey> {
let mut zero_lamport_key_clone = Measure::start("zero_lamport_key");
let pubkeys = self.accounts_index.zero_lamport_pubkeys().clone();
let max_slot = max_clean_root.unwrap_or_else(|| self.accounts_index.max_root());
let dirty_stores: Vec<(Slot, Arc<AccountStorageEntry>)> = self
.dirty_stores
.iter()
.filter_map(|entry| {
if entry.key().0 <= max_slot {
Some((entry.key().0, entry.value().clone()))
} else {
None
}
})
.collect();
self.dirty_stores
.retain(|(slot, _store_id), _store_ref| *slot > max_slot);
let dirty_stores_len = dirty_stores.len();
let pubkeys = DashSet::new();
for (_slot, store) in dirty_stores {
for account in store.accounts.accounts(0) {
pubkeys.insert(account.meta.pubkey);
}
}
trace!(
"dirty_stores.len: {} pubkeys.len: {}",
dirty_stores_len,
pubkeys.len()
);
timings.zero_lamport_count = pubkeys.len() as u64;
zero_lamport_key_clone.stop();
timings.zero_lamport_key_clone_us += zero_lamport_key_clone.as_us();

let mut collect_delta_keys = Measure::start("key_create");
let max_slot = max_clean_root.unwrap_or_else(|| self.accounts_index.max_root());
let delta_keys = self.remove_uncleaned_slots_and_collect_pubkeys_up_to_slot(max_slot);
collect_delta_keys.stop();
timings.collect_delta_keys_us += collect_delta_keys.as_us();
Expand Down Expand Up @@ -1685,16 +1712,6 @@ impl AccountsDb {
self.accounts_index
.roots_and_ref_count(&locked_entry, max_clean_root),
);
} else {
// prune zero_lamport_pubkey set which should contain all 0-lamport
// keys whether rooted or not. A 0-lamport update may become rooted
// in the future.
if !slot_list
.iter()
.any(|(_slot, account_info)| account_info.lamports == 0)
{
self.accounts_index.remove_zero_lamport_key(pubkey);
}
}
// Release the lock
let slot = *slot;
Expand All @@ -1718,11 +1735,7 @@ impl AccountsDb {
// touched in must be unrooted.
purges_old_accounts.push(*pubkey);
}
AccountIndexGetResult::Missing(lock) => {
// pubkey is missing from index, so remove from zero_lamports_list
self.accounts_index.remove_zero_lamport_key(pubkey);
drop(lock);
}
AccountIndexGetResult::Missing(_lock) => {}
};
}
(purges_zero_lamports, purges_old_accounts)
Expand Down Expand Up @@ -4799,6 +4812,8 @@ impl AccountsDb {
store.slot(), *slot
);
let count = store.remove_account(account_info.stored_size, reset_accounts);
self.dirty_stores
.insert((*slot, store.append_vec_id()), store.clone());
if count == 0 {
dead_slots.insert(*slot);
} else if self.caching_enabled
Expand Down Expand Up @@ -5320,6 +5335,11 @@ impl AccountsDb {
if self.caching_enabled {
self.accounts_cache.add_root(slot);
}
if let Some(slot_stores) = self.storage.get_slot_stores(slot) {
for (store_id, store) in slot_stores.read().unwrap().iter() {
self.dirty_stores.insert((slot, *store_id), store.clone());
}
}
}

pub fn get_snapshot_storages(
Expand Down Expand Up @@ -7096,6 +7116,7 @@ pub mod tests {

accounts.clean_accounts(None, false);
// Slot 2 will now be cleaned, which will leave account 1 with a ref count of 0
// TODO: fix these
assert!(accounts.storage.get_slot_stores(2).is_none());
assert_eq!(accounts.accounts_index.ref_count_from_storage(&pubkey1), 0);
}
Expand Down Expand Up @@ -9248,6 +9269,54 @@ pub mod tests {
assert!(total_len < STORE_META_OVERHEAD);
}

#[test]
fn test_store_clean_after_shrink() {
solana_logger::setup();
let accounts = AccountsDb::new_with_config(
vec![],
&ClusterType::Development,
AccountSecondaryIndexes::default(),
true,
);

let account = AccountSharedData::new(1, 16 * 4096, &Pubkey::default());
let pubkey1 = solana_sdk::pubkey::new_rand();
accounts.store_cached(0, &[(&pubkey1, &account)]);

let pubkey2 = solana_sdk::pubkey::new_rand();
accounts.store_cached(0, &[(&pubkey2, &account)]);

let zero_account = AccountSharedData::new(0, 1, &Pubkey::default());
accounts.store_cached(1, &[(&pubkey1, &zero_account)]);

// Add root 0 and flush separately
accounts.get_accounts_delta_hash(0);
accounts.add_root(0);
accounts.flush_accounts_cache(true, None);

// clear out the dirty keys
accounts.clean_accounts(None, false);

// flush 1
accounts.get_accounts_delta_hash(1);
accounts.add_root(1);
accounts.flush_accounts_cache(true, None);

accounts.print_accounts_stats("pre-clean");

// clean to remove pubkey1 from 0,
// shrink to shrink pubkey1 from 0
// then another clean to remove pubkey1 from slot 1
accounts.clean_accounts(None, false);

accounts.shrink_candidate_slots();

accounts.clean_accounts(None, false);

accounts.print_accounts_stats("post-clean");
assert_eq!(accounts.accounts_index.ref_count_from_storage(&pubkey1), 0);
}

#[test]
fn test_store_reuse() {
solana_logger::setup();
Expand All @@ -9272,6 +9341,10 @@ pub mod tests {
accounts.add_root(1);
accounts.clean_accounts(None, false);
accounts.shrink_all_slots(false);

// Clean again to flush the dirty stores
// and allow them to be recycled in the next step
accounts.clean_accounts(None, false);
accounts.print_accounts_stats("post-shrink");
let num_stores = accounts.recycle_stores.read().unwrap().entry_count();
assert!(num_stores > 0);
Expand Down
21 changes: 0 additions & 21 deletions runtime/src/accounts_index.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use crate::{
secondary_index::*,
};
use bv::BitVec;
use dashmap::DashSet;
use log::*;
use ouroboros::self_referencing;
use solana_measure::measure::Measure;
Expand Down Expand Up @@ -570,7 +569,6 @@ pub struct AccountsIndex<T> {
spl_token_owner_index: SecondaryIndex<RwLockSecondaryIndexEntry>,
roots_tracker: RwLock<RootsTracker>,
ongoing_scan_roots: RwLock<BTreeMap<Slot, u64>>,
zero_lamport_pubkeys: DashSet<Pubkey>,
}

impl<T> Default for AccountsIndex<T> {
Expand All @@ -588,7 +586,6 @@ impl<T> Default for AccountsIndex<T> {
),
roots_tracker: RwLock::<RootsTracker>::default(),
ongoing_scan_roots: RwLock::<BTreeMap<Slot, u64>>::default(),
zero_lamport_pubkeys: DashSet::<Pubkey>::default(),
}
}
}
Expand Down Expand Up @@ -1277,9 +1274,6 @@ impl<T: 'static + Clone + IsCached + ZeroLamport> AccountsIndex<T> {
&mut w_account_maps,
new_item,
);
if account_info.is_zero_lamport() {
self.zero_lamport_pubkeys.insert(*pubkey);
}
if let Some(mut w_account_entry) = account_entry {
w_account_entry.update(slot, account_info, &mut _reclaims);
true
Expand Down Expand Up @@ -1317,9 +1311,6 @@ impl<T: 'static + Clone + IsCached + ZeroLamport> AccountsIndex<T> {
// - The secondary index is never consulted as primary source of truth for gets/stores.
// So, what the accounts_index sees alone is sufficient as a source of truth for other non-scan
// account operations.
if account_info.is_zero_lamport() {
self.zero_lamport_pubkeys.insert(*pubkey);
}
if let Some(mut w_account_entry) = w_account_entry {
w_account_entry.update(slot, account_info, reclaims);
false
Expand All @@ -1331,14 +1322,6 @@ impl<T: 'static + Clone + IsCached + ZeroLamport> AccountsIndex<T> {
is_newly_inserted
}

pub fn remove_zero_lamport_key(&self, pubkey: &Pubkey) {
self.zero_lamport_pubkeys.remove(pubkey);
}

pub fn zero_lamport_pubkeys(&self) -> &DashSet<Pubkey> {
&self.zero_lamport_pubkeys
}

pub fn unref_from_storage(&self, pubkey: &Pubkey) {
if let Some(locked_entry) = self.get_account_read_entry(pubkey) {
locked_entry.unref();
Expand Down Expand Up @@ -2418,8 +2401,6 @@ pub mod tests {
let items = vec![(pubkey, account_info)];
index.insert_new_if_missing_into_primary_index(slot, items);

assert!(index.zero_lamport_pubkeys().is_empty());

let mut ancestors = Ancestors::default();
assert!(index.get(&pubkey, Some(&ancestors), None).is_none());
assert!(index.get(&pubkey, None, None).is_none());
Expand All @@ -2439,8 +2420,6 @@ pub mod tests {
let items = vec![(pubkey, account_info)];
index.insert_new_if_missing_into_primary_index(slot, items);

assert!(!index.zero_lamport_pubkeys().is_empty());

let mut ancestors = Ancestors::default();
assert!(index.get(&pubkey, Some(&ancestors), None).is_none());
assert!(index.get(&pubkey, None, None).is_none());
Expand Down

0 comments on commit ef98258

Please sign in to comment.