From 6281c56095a386b264bdc9bb839767e702a6ecc0 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Wed, 30 Sep 2020 15:45:29 -0700 Subject: [PATCH 1/6] Fix clean removing purged AppendVec --- runtime/src/accounts_db.rs | 186 +++++++++++++++++++++++++++++++------ 1 file changed, 156 insertions(+), 30 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 8b75b6811ea507..ce57154910bc7e 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -540,7 +540,11 @@ impl AccountsDB { // Reclaim older states of rooted non-zero lamport accounts as a general // AccountsDB bloat mitigation and preprocess for better zero-lamport purging. - fn clean_old_rooted_accounts(&self, purges_in_root: Vec, max_clean_root: Option) { + fn clean_old_rooted_accounts( + &self, + purges_in_root: Vec, + max_clean_root: Option, + ) -> HashSet { // This number isn't carefully chosen; just guessed randomly such that // the hot loop will be the order of ~Xms. const INDEX_CLEAN_BULK_COUNT: usize = 4096; @@ -562,10 +566,11 @@ impl AccountsDB { inc_new_counter_info!("clean-old-root-par-clean-ms", clean_rooted.as_ms() as usize); let mut measure = Measure::start("clean_old_root_reclaims"); - self.handle_reclaims(&reclaims, None, false); + let dead_slots = self.handle_reclaims(&reclaims, None, false); measure.stop(); debug!("{} {}", clean_rooted, measure); inc_new_counter_info!("clean-old-root-reclaim-ms", measure.as_ms() as usize); + dead_slots } fn do_reset_uncleaned_roots( @@ -681,7 +686,8 @@ impl AccountsDB { let (slot, account_info) = &list[index]; if account_info.lamports == 0 { purges.insert(*pubkey, accounts_index.would_purge(pubkey)); - } else if accounts_index.uncleaned_roots.contains(slot) { + } + if accounts_index.uncleaned_roots.contains(slot) { purges_in_root.push(*pubkey); } } @@ -702,9 +708,13 @@ impl AccountsDB { accounts_scan.stop(); let mut clean_old_rooted = Measure::start("clean_old_roots"); - if !purges_in_root.is_empty() { - self.clean_old_rooted_accounts(purges_in_root, max_clean_root); - } + let old_rooted_dead_slots = { + if !purges_in_root.is_empty() { + self.clean_old_rooted_accounts(purges_in_root, max_clean_root) + } else { + HashSet::new() + } + }; self.do_reset_uncleaned_roots(&mut candidates, max_clean_root); clean_old_rooted.stop(); @@ -713,26 +723,35 @@ impl AccountsDB { // Calculate store counts as if everything was purged // Then purge if we can let mut store_counts: HashMap)> = HashMap::new(); - let storage = self.storage.read().unwrap(); - for (key, (account_infos, _ref_count)) in &purges { - for (slot, account_info) in account_infos { - let slot_storage = storage.0.get(&slot).unwrap(); - let store = slot_storage.get(&account_info.store_id).unwrap(); + for (key, (account_infos, ref_count)) in purges.iter_mut() { + account_infos.retain(|(slot, account_info)| { + if old_rooted_dead_slots.contains(slot) { + *ref_count = self + .accounts_index + .read() + .unwrap() + .ref_count_from_storage(&key); + return false; + } if let Some(store_count) = store_counts.get_mut(&account_info.store_id) { store_count.0 -= 1; store_count.1.insert(*key); } else { let mut key_set = HashSet::new(); key_set.insert(*key); - store_counts.insert( - account_info.store_id, - (store.count_and_status.read().unwrap().0 - 1, key_set), - ); + let count: usize = { + let storage = self.storage.read().unwrap(); + let slot_storage = storage.0.get(&slot).unwrap(); + let store = slot_storage.get(&account_info.store_id).unwrap(); + let count = store.count_and_status.read().unwrap().0; + count - 1 + }; + store_counts.insert(account_info.store_id, (count, key_set)); } - } + true + }); } store_counts_time.stop(); - drop(storage); let mut calc_deps_time = Measure::start("calc_deps"); Self::calc_delete_dependencies(&purges, &mut store_counts); @@ -816,21 +835,24 @@ impl AccountsDB { reclaims: SlotSlice, expected_single_dead_slot: Option, no_dead_slot: bool, - ) { - if !reclaims.is_empty() { - let dead_slots = self.remove_dead_accounts(reclaims, expected_single_dead_slot); - if no_dead_slot { - assert!(dead_slots.is_empty()); - } else if let Some(expected_single_dead_slot) = expected_single_dead_slot { - assert!(dead_slots.len() <= 1); - if dead_slots.len() == 1 { - assert!(dead_slots.contains(&expected_single_dead_slot)); - } - } - if !dead_slots.is_empty() { - self.process_dead_slots(&dead_slots); + ) -> HashSet { + if reclaims.is_empty() { + return HashSet::new(); + } + + let dead_slots = self.remove_dead_accounts(reclaims, expected_single_dead_slot); + if no_dead_slot { + assert!(dead_slots.is_empty()); + } else if let Some(expected_single_dead_slot) = expected_single_dead_slot { + assert!(dead_slots.len() <= 1); + if dead_slots.len() == 1 { + assert!(dead_slots.contains(&expected_single_dead_slot)); } } + if !dead_slots.is_empty() { + self.process_dead_slots(&dead_slots); + } + dead_slots } // Must be kept private!, does sensitive cleanup that should only be called from @@ -3040,6 +3062,110 @@ pub mod tests { } } + #[test] + fn test_clean_zero_lamport_and_dead_slot() { + solana_logger::setup(); + + let accounts = AccountsDB::new(Vec::new(), &ClusterType::Development); + let pubkey1 = Pubkey::new_rand(); + let pubkey2 = Pubkey::new_rand(); + let account = Account::new(1, 1, &Account::default().owner); + let zero_lamport_account = Account::new(0, 0, &Account::default().owner); + + // Store two accounts + accounts.store(0, &[(&pubkey1, &account)]); + accounts.store(0, &[(&pubkey2, &account)]); + + // Make sure both accounts are in the same AppendVec in slot 0, which + // will prevent pubkey1 from being cleaned up later even when it's a + // zero-lamport account + let ancestors: HashMap = vec![(0, 1)].into_iter().collect(); + let (slot1, account_info1) = accounts + .accounts_index + .read() + .unwrap() + .get(&pubkey1, Some(&ancestors), None) + .map(|(account_list1, index1)| account_list1[index1].clone()) + .unwrap(); + let (slot2, account_info2) = accounts + .accounts_index + .read() + .unwrap() + .get(&pubkey2, Some(&ancestors), None) + .map(|(account_list2, index2)| account_list2[index2].clone()) + .unwrap(); + assert_eq!(slot1, 0); + assert_eq!(slot1, slot2); + assert_eq!(account_info1.store_id, account_info2.store_id); + + // Update account 1 in slot 1 + accounts.store(1, &[(&pubkey1, &account)]); + + // Update account 1 as zero lamports account + accounts.store(2, &[(&pubkey1, &zero_lamport_account)]); + + // Pubkey 1 was the only account in slot 1, and it was updated in slot 2, so + // slot 1 should be purged + accounts.add_root(0); + accounts.add_root(1); + accounts.add_root(2); + + // Slot 1 should be removed, slot 0 cannot be removed because it still has + // the latest update for pubkey 2 + accounts.clean_accounts(None); + assert!(accounts.storage.read().unwrap().0.get(&0).is_some()); + assert!(accounts.storage.read().unwrap().0.get(&1).is_none()); + + // Slot 1 should be cleaned because all it's accounts are + // zero lamports, and are not present in any other slot's + // storage entries + assert_eq!(accounts.alive_account_count_in_store(1), 0); + } + + #[test] + fn test_clean_zero_lamport_and_old_roots() { + solana_logger::setup(); + + let accounts = AccountsDB::new(Vec::new(), &ClusterType::Development); + let pubkey = Pubkey::new_rand(); + let account = Account::new(1, 0, &Account::default().owner); + let zero_lamport_account = Account::new(0, 0, &Account::default().owner); + + // Store a zero-lamport account + accounts.store(0, &[(&pubkey, &account)]); + accounts.store(1, &[(&pubkey, &zero_lamport_account)]); + + // Simulate rooting the zero-lamport account, should be a + // candidate for cleaning + accounts.add_root(0); + accounts.add_root(1); + + // Slot 0 should be removed, and + // zero-lamport account should be cleaned + accounts.clean_accounts(None); + + assert!(accounts.storage.read().unwrap().0.get(&0).is_none()); + assert!(accounts.storage.read().unwrap().0.get(&1).is_none()); + + // Slot 0 should be cleaned because all it's accounts have been + // updated in the rooted slot 1 + assert_eq!(accounts.alive_account_count_in_store(0), 0); + + // Slot 1 should be cleaned because all it's accounts are + // zero lamports, and are not present in any other slot's + // storage entries + assert_eq!(accounts.alive_account_count_in_store(1), 0); + + // zero lamport account, should no longer exist in accounts index + // because it has been removed + assert!(accounts + .accounts_index + .read() + .unwrap() + .get(&pubkey, None, None) + .is_none()); + } + #[test] fn test_clean_old_with_normal_account() { solana_logger::setup(); From b40511b4939bf2ffaa3a1df52dff75490f10a4c1 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Thu, 1 Oct 2020 00:23:43 -0700 Subject: [PATCH 2/6] Clarification --- runtime/src/accounts_db.rs | 67 +++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 5 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index ce57154910bc7e..99e2b90430be47 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -688,6 +688,11 @@ impl AccountsDB { purges.insert(*pubkey, accounts_index.would_purge(pubkey)); } if accounts_index.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) = max_clean_root { + assert!(*slot <= max_clean_root); + } purges_in_root.push(*pubkey); } } @@ -724,13 +729,10 @@ impl AccountsDB { // Then purge if we can let mut store_counts: HashMap)> = HashMap::new(); for (key, (account_infos, ref_count)) in purges.iter_mut() { + let mut ref_count_needs_update = false; account_infos.retain(|(slot, account_info)| { if old_rooted_dead_slots.contains(slot) { - *ref_count = self - .accounts_index - .read() - .unwrap() - .ref_count_from_storage(&key); + ref_count_needs_update = true; return false; } if let Some(store_count) = store_counts.get_mut(&account_info.store_id) { @@ -750,6 +752,14 @@ impl AccountsDB { } true }); + + if ref_count_needs_update { + *ref_count = self + .accounts_index + .read() + .unwrap() + .ref_count_from_storage(&key); + } } store_counts_time.stop(); @@ -3269,6 +3279,53 @@ pub mod tests { .is_none()); } + #[test] + fn test_clean_max_slot_zero_lamport_account() { + solana_logger::setup(); + + let accounts = AccountsDB::new(Vec::new(), &ClusterType::Development); + let pubkey = Pubkey::new_rand(); + let account = Account::new(1, 0, &Account::default().owner); + let zero_account = Account::new(0, 0, &Account::default().owner); + + // store an account, make it a zero lamport account + // in slot 1 + accounts.store(0, &[(&pubkey, &account)]); + accounts.store(1, &[(&pubkey, &zero_account)]); + + // simulate slots are rooted after while + accounts.add_root(0); + accounts.add_root(1); + + // Only clean up to account 0, should not purge slot 0 based on + // updates in later slots in slot 1 + assert_eq!(accounts.alive_account_count_in_store(0), 1); + assert_eq!(accounts.alive_account_count_in_store(1), 1); + accounts.clean_accounts(Some(0)); + assert_eq!(accounts.alive_account_count_in_store(0), 1); + assert_eq!(accounts.alive_account_count_in_store(1), 1); + assert!(accounts + .accounts_index + .read() + .unwrap() + .get(&pubkey, None, None) + .is_some()); + + // Now the account can be cleaned up + accounts.clean_accounts(Some(1)); + assert_eq!(accounts.alive_account_count_in_store(0), 0); + assert_eq!(accounts.alive_account_count_in_store(1), 0); + + // The zero lamport account, should no longer exist in accounts index + // because it has been removed + assert!(accounts + .accounts_index + .read() + .unwrap() + .get(&pubkey, None, None) + .is_none()); + } + #[test] fn test_uncleaned_roots_with_account() { solana_logger::setup(); From 101eef56f7a5077cddb28ae1de4442d425eecc70 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Thu, 1 Oct 2020 15:03:20 -0700 Subject: [PATCH 3/6] Fix tests --- runtime/src/accounts_db.rs | 62 +++++++++++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 14 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 99e2b90430be47..49302936ded4c3 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -544,7 +544,7 @@ impl AccountsDB { &self, purges_in_root: Vec, max_clean_root: Option, - ) -> HashSet { + ) -> (HashSet, HashMap>) { // This number isn't carefully chosen; just guessed randomly such that // the hot loop will be the order of ~Xms. const INDEX_CLEAN_BULK_COUNT: usize = 4096; @@ -566,11 +566,11 @@ impl AccountsDB { inc_new_counter_info!("clean-old-root-par-clean-ms", clean_rooted.as_ms() as usize); let mut measure = Measure::start("clean_old_root_reclaims"); - let dead_slots = self.handle_reclaims(&reclaims, None, false); + let (dead_slots, removed_accounts) = self.handle_reclaims(&reclaims, None, false); measure.stop(); debug!("{} {}", clean_rooted, measure); inc_new_counter_info!("clean-old-root-reclaim-ms", measure.as_ms() as usize); - dead_slots + (dead_slots, removed_accounts) } fn do_reset_uncleaned_roots( @@ -713,11 +713,11 @@ impl AccountsDB { accounts_scan.stop(); let mut clean_old_rooted = Measure::start("clean_old_roots"); - let old_rooted_dead_slots = { + let (old_rooted_dead_slots, removed_accounts) = { if !purges_in_root.is_empty() { self.clean_old_rooted_accounts(purges_in_root, max_clean_root) } else { - HashSet::new() + (HashSet::new(), HashMap::new()) } }; self.do_reset_uncleaned_roots(&mut candidates, max_clean_root); @@ -732,9 +732,20 @@ impl AccountsDB { let mut ref_count_needs_update = false; account_infos.retain(|(slot, account_info)| { if old_rooted_dead_slots.contains(slot) { + // ref count is only updated when a slot is dead in + // `clean_dead_slots()` ref_count_needs_update = true; return false; } + // Check if this update in `slot`to the account with `key` was reclaimed earlier by + // `clean_old_rooted_accounts()` + let was_reclaimed = removed_accounts + .get(&account_info.store_id) + .map(|store_removed| store_removed.contains(&account_info.offset)) + .unwrap_or(false); + if was_reclaimed { + return false; + } if let Some(store_count) = store_counts.get_mut(&account_info.store_id) { store_count.0 -= 1; store_count.1.insert(*key); @@ -845,12 +856,13 @@ impl AccountsDB { reclaims: SlotSlice, expected_single_dead_slot: Option, no_dead_slot: bool, - ) -> HashSet { + ) -> (HashSet, HashMap>) { if reclaims.is_empty() { - return HashSet::new(); + return (HashSet::new(), HashMap::new()); } - let dead_slots = self.remove_dead_accounts(reclaims, expected_single_dead_slot); + let (dead_slots, removed_accounts) = + self.remove_dead_accounts(reclaims, expected_single_dead_slot); if no_dead_slot { assert!(dead_slots.is_empty()); } else if let Some(expected_single_dead_slot) = expected_single_dead_slot { @@ -862,7 +874,7 @@ impl AccountsDB { if !dead_slots.is_empty() { self.process_dead_slots(&dead_slots); } - dead_slots + (dead_slots, removed_accounts) } // Must be kept private!, does sensitive cleanup that should only be called from @@ -2069,10 +2081,15 @@ impl AccountsDB { &self, reclaims: SlotSlice, expected_slot: Option, - ) -> HashSet { + ) -> (HashSet, HashMap>) { let storage = self.storage.read().unwrap(); let mut dead_slots = HashSet::new(); + let mut removed_accounts: HashMap> = HashMap::new(); for (slot, account_info) in reclaims { + removed_accounts + .entry(account_info.store_id) + .or_default() + .insert(account_info.offset); if let Some(expected_slot) = expected_slot { assert_eq!(*slot, expected_slot); } @@ -2101,7 +2118,7 @@ impl AccountsDB { true }); - dead_slots + (dead_slots, removed_accounts) } fn clean_dead_slots(&self, dead_slots: &HashSet) { @@ -3227,8 +3244,8 @@ pub mod tests { accounts.clean_accounts(None); - //still old state behind zero-lamport account isn't cleaned up - assert_eq!(accounts.alive_account_count_in_store(0), 1); + //Old state behind zero-lamport account is cleaned up + assert_eq!(accounts.alive_account_count_in_store(0), 0); assert_eq!(accounts.alive_account_count_in_store(1), 2); } @@ -3511,11 +3528,28 @@ pub mod tests { let mut current_slot = 1; accounts.store(current_slot, &[(&pubkey, &account)]); - // Store another live account to slot 1 which will prevent any purge // since the store count will not be zero accounts.store(current_slot, &[(&pubkey2, &account2)]); accounts.add_root(current_slot); + let (slot1, account_info1) = accounts + .accounts_index + .read() + .unwrap() + .get(&pubkey, None, None) + .map(|(account_list1, index1)| account_list1[index1].clone()) + .unwrap(); + let (slot2, account_info2) = accounts + .accounts_index + .read() + .unwrap() + .get(&pubkey2, None, None) + .map(|(account_list2, index2)| account_list2[index2].clone()) + .unwrap(); + assert_eq!(slot1, current_slot); + assert_eq!(slot1, slot2); + assert_eq!(account_info1.store_id, account_info2.store_id); + println!("stored into {}", account_info1.store_id); current_slot += 1; accounts.store(current_slot, &[(&pubkey, &zero_lamport_account)]); From 8579a1100cb051350d10773aa394d0570a6dfb70 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Thu, 1 Oct 2020 17:23:33 -0700 Subject: [PATCH 4/6] Track purged keys --- runtime/src/accounts_db.rs | 175 +++++++++++++++++++++++-------------- 1 file changed, 107 insertions(+), 68 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 49302936ded4c3..7084485bed1291 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -100,6 +100,9 @@ pub type SnapshotStorages = Vec; // Each slot has a set of storage entries. pub(crate) type SlotStores = HashMap>; +type PurgedAccountSlots = HashMap>; +type ReclaimedAccounts = HashMap>; + trait Versioned { fn version(&self) -> u64; } @@ -544,7 +547,7 @@ impl AccountsDB { &self, purges_in_root: Vec, max_clean_root: Option, - ) -> (HashSet, HashMap>) { + ) -> (PurgedAccountSlots, ReclaimedAccounts) { // This number isn't carefully chosen; just guessed randomly such that // the hot loop will be the order of ~Xms. const INDEX_CLEAN_BULK_COUNT: usize = 4096; @@ -566,11 +569,11 @@ impl AccountsDB { inc_new_counter_info!("clean-old-root-par-clean-ms", clean_rooted.as_ms() as usize); let mut measure = Measure::start("clean_old_root_reclaims"); - let (dead_slots, removed_accounts) = self.handle_reclaims(&reclaims, None, false); + let (purged_account_slots, removed_accounts) = self.handle_reclaims(&reclaims, None, false); measure.stop(); debug!("{} {}", clean_rooted, measure); inc_new_counter_info!("clean-old-root-reclaim-ms", measure.as_ms() as usize); - (dead_slots, removed_accounts) + (purged_account_slots, removed_accounts) } fn do_reset_uncleaned_roots( @@ -599,12 +602,30 @@ impl AccountsDB { // do not match the criteria of deleting all appendvecs which contain them // then increment their storage count. let mut already_counted = HashSet::new(); - for (_pubkey, (account_infos, ref_count_from_storage)) in purges.iter() { + for (pubkey, (account_infos, ref_count_from_storage)) in purges.iter() { let no_delete = if account_infos.len() as u64 != *ref_count_from_storage { + debug!( + "calc_delete_dependencies(), + pubkey: {}, + account_infos: {:?}, + account_infos_len: {}, + ref_count_from_storage: {}", + pubkey, + account_infos, + account_infos.len(), + ref_count_from_storage, + ); true } else { let mut no_delete = false; for (_slot, account_info) in account_infos { + debug!( + "calc_delete_dependencies() + storage id: {}, + count len: {}", + account_info.store_id, + store_counts.get(&account_info.store_id).unwrap().0, + ); if store_counts.get(&account_info.store_id).unwrap().0 != 0 { no_delete = true; break; @@ -685,6 +706,7 @@ impl AccountsDB { if let Some((list, index)) = accounts_index.get(pubkey, None, max_clean_root) { let (slot, account_info) = &list[index]; if account_info.lamports == 0 { + debug!("purging zero lamport {}, slot: {}", pubkey, slot); purges.insert(*pubkey, accounts_index.would_purge(pubkey)); } if accounts_index.uncleaned_roots.contains(slot) { @@ -693,6 +715,7 @@ impl AccountsDB { if let Some(max_clean_root) = max_clean_root { assert!(*slot <= max_clean_root); } + debug!("purging uncleaned {}, slot: {}", pubkey, slot); purges_in_root.push(*pubkey); } } @@ -713,11 +736,11 @@ impl AccountsDB { accounts_scan.stop(); let mut clean_old_rooted = Measure::start("clean_old_roots"); - let (old_rooted_dead_slots, removed_accounts) = { + let (purged_account_slots, removed_accounts) = { if !purges_in_root.is_empty() { self.clean_old_rooted_accounts(purges_in_root, max_clean_root) } else { - (HashSet::new(), HashMap::new()) + (HashMap::new(), HashMap::new()) } }; self.do_reset_uncleaned_roots(&mut candidates, max_clean_root); @@ -729,15 +752,24 @@ impl AccountsDB { // Then purge if we can let mut store_counts: HashMap)> = HashMap::new(); for (key, (account_infos, ref_count)) in purges.iter_mut() { - let mut ref_count_needs_update = false; + if purged_account_slots.contains_key(&key) { + *ref_count = self + .accounts_index + .read() + .unwrap() + .ref_count_from_storage(&key); + } account_infos.retain(|(slot, account_info)| { - if old_rooted_dead_slots.contains(slot) { - // ref count is only updated when a slot is dead in - // `clean_dead_slots()` - ref_count_needs_update = true; + let was_slot_purged = purged_account_slots + .get(&key) + .map(|slots_removed| slots_removed.contains(slot)) + .unwrap_or(false); + if was_slot_purged { + // No need to look up the slot storage below if the entire + // slot was purged return false; } - // Check if this update in `slot`to the account with `key` was reclaimed earlier by + // Check if this update in `slot` to the account with `key` was reclaimed earlier by // `clean_old_rooted_accounts()` let was_reclaimed = removed_accounts .get(&account_info.store_id) @@ -759,18 +791,14 @@ impl AccountsDB { let count = store.count_and_status.read().unwrap().0; count - 1 }; + debug!( + "store_counts, inserting slot: {}, store id: {}, count: {}", + slot, account_info.store_id, count + ); store_counts.insert(account_info.store_id, (count, key_set)); } true }); - - if ref_count_needs_update { - *ref_count = self - .accounts_index - .read() - .unwrap() - .ref_count_from_storage(&key); - } } store_counts_time.stop(); @@ -856,9 +884,9 @@ impl AccountsDB { reclaims: SlotSlice, expected_single_dead_slot: Option, no_dead_slot: bool, - ) -> (HashSet, HashMap>) { + ) -> (PurgedAccountSlots, ReclaimedAccounts) { if reclaims.is_empty() { - return (HashSet::new(), HashMap::new()); + return (HashMap::new(), HashMap::new()); } let (dead_slots, removed_accounts) = @@ -871,17 +899,19 @@ impl AccountsDB { assert!(dead_slots.contains(&expected_single_dead_slot)); } } - if !dead_slots.is_empty() { - self.process_dead_slots(&dead_slots); - } - (dead_slots, removed_accounts) + let purged_account_slots = if !dead_slots.is_empty() { + self.process_dead_slots(&dead_slots) + } else { + HashMap::new() + }; + (purged_account_slots, removed_accounts) } // Must be kept private!, does sensitive cleanup that should only be called from // supported pipelines in AccountsDb - fn process_dead_slots(&self, dead_slots: &HashSet) { + fn process_dead_slots(&self, dead_slots: &HashSet) -> PurgedAccountSlots { let mut clean_dead_slots = Measure::start("reclaims::purge_slots"); - self.clean_dead_slots(&dead_slots); + let purged_account_slots = self.clean_dead_slots(&dead_slots); clean_dead_slots.stop(); let mut purge_slots = Measure::start("reclaims::purge_slots"); @@ -889,11 +919,13 @@ impl AccountsDB { purge_slots.stop(); debug!( - "process_dead_slots({}): {} {}", + "process_dead_slots({}): {} {} {:?}", dead_slots.len(), clean_dead_slots, - purge_slots + purge_slots, + dead_slots, ); + purged_account_slots } fn do_shrink_stale_slot(&self, slot: Slot) -> usize { @@ -2081,10 +2113,10 @@ impl AccountsDB { &self, reclaims: SlotSlice, expected_slot: Option, - ) -> (HashSet, HashMap>) { + ) -> (HashSet, ReclaimedAccounts) { let storage = self.storage.read().unwrap(); let mut dead_slots = HashSet::new(); - let mut removed_accounts: HashMap> = HashMap::new(); + let mut removed_accounts: ReclaimedAccounts = HashMap::new(); for (slot, account_info) in reclaims { removed_accounts .entry(account_info.store_id) @@ -2121,7 +2153,8 @@ impl AccountsDB { (dead_slots, removed_accounts) } - fn clean_dead_slots(&self, dead_slots: &HashSet) { + fn clean_dead_slots(&self, dead_slots: &HashSet) -> PurgedAccountSlots { + let mut purged_account_slots: PurgedAccountSlots = HashMap::new(); { let mut measure = Measure::start("clean_dead_slots-ms"); let storage = self.storage.read().unwrap(); @@ -2153,7 +2186,8 @@ impl AccountsDB { }) }; let index = self.accounts_index.read().unwrap(); - for (_slot, pubkey) in slot_pubkeys { + for (slot, pubkey) in slot_pubkeys { + purged_account_slots.entry(pubkey).or_default().insert(slot); index.unref_from_storage(&pubkey); } drop(index); @@ -2171,6 +2205,7 @@ impl AccountsDB { bank_hashes.remove(slot); } } + purged_account_slots } fn hash_accounts( @@ -3402,16 +3437,17 @@ pub mod tests { // CREATE SLOT 1 let latest_slot = 1; - // Modify the first 10 of the slot 0 accounts as updates in slot 1 + // Modify the first 10 of the accounts from slot 0 in slot 1 modify_accounts(&accounts, &pubkeys, latest_slot, 10, 3); + // Overwrite account 30 from slot 0 with lamports=0 into slot 1. + // Slot 1 should now have 10 + 1 = 11 accounts + let account = Account::new(0, 0, &Account::default().owner); + accounts.store(latest_slot, &[(&pubkeys[30], &account)]); - // Create 10 new accounts in slot 1 + // Create 10 new accounts in slot 1, should now have 11 = 10 = 21 + // accounts create_account(&accounts, &mut pubkeys1, latest_slot, 10, 0, 0); - // Store a lamports=0 account in slot 1. Slot 1 should now have - // 10 + 10 + 1 = 21 accounts - let account = Account::new(0, 0, &Account::default().owner); - accounts.store(latest_slot, &[(&pubkeys[30], &account)]); accounts.add_root(latest_slot); assert!(check_storage(&accounts, 1, 21)); @@ -3419,24 +3455,26 @@ pub mod tests { let latest_slot = 2; let mut pubkeys2: Vec = vec![]; - // Modify original slot 0 accounts in slot 2 + // Modify first 20 of the accounts from slot 0 in slot 2 modify_accounts(&accounts, &pubkeys, latest_slot, 20, 4); accounts.clean_accounts(None); + // Overwrite account 31 from slot 0 with lamports=0 into slot 2. + // Slot 2 should now have 20 + 1 = 21 accounts + let account = Account::new(0, 0, &Account::default().owner); + accounts.store(latest_slot, &[(&pubkeys[31], &account)]); - // Create 10 new accounts in slot 2 + // Create 10 new accounts in slot 2. Slot 2 should now have + // 21 + 10 = 31 accounts create_account(&accounts, &mut pubkeys2, latest_slot, 10, 0, 0); - // Store a lamports=0 account in slot 2. Slot 2 should now have - // 10 + 10 + 10 + 1 = 31 accounts - let account = Account::new(0, 0, &Account::default().owner); - accounts.store(latest_slot, &[(&pubkeys[31], &account)]); accounts.add_root(latest_slot); assert!(check_storage(&accounts, 2, 31)); accounts.clean_accounts(None); - // The first 20 accounts have been modified in slot 2, so only - // 80 accounts left in slot 0. - assert!(check_storage(&accounts, 0, 80)); + // The first 20 accounts of slot 0 have been updated in slot 2, as well as + // accounts 30 and 31 (overwritten with zero-lamport accounts in slot 1 and + // slot 2 respectively), so only 78 accounts are left in slot 0's storage entries. + assert!(check_storage(&accounts, 0, 78)); // 10 of the 21 accounts have been modified in slot 2, so only 11 // accounts left in slot 1. assert!(check_storage(&accounts, 1, 11)); @@ -3526,6 +3564,7 @@ pub mod tests { let accounts = AccountsDB::new_single(); accounts.add_root(0); + // Step A let mut current_slot = 1; accounts.store(current_slot, &[(&pubkey, &account)]); // Store another live account to slot 1 which will prevent any purge @@ -3549,9 +3588,10 @@ pub mod tests { assert_eq!(slot1, current_slot); assert_eq!(slot1, slot2); assert_eq!(account_info1.store_id, account_info2.store_id); - println!("stored into {}", account_info1.store_id); + // Step B current_slot += 1; + let zero_lamport_slot = current_slot; accounts.store(current_slot, &[(&pubkey, &zero_lamport_account)]); accounts.add_root(current_slot); @@ -3566,24 +3606,23 @@ pub mod tests { accounts.print_accounts_stats("post_purge"); - // Make sure the index is not touched - assert_eq!( - accounts - .accounts_index - .read() - .unwrap() - .account_maps - .get(&pubkey) - .unwrap() - .1 - .read() - .unwrap() - .len(), - 2 - ); - - // slot 1 & 2 should have stores - check_storage(&accounts, 1, 2); + // The earlier entry for pubkey in the account index is purged, + let (slot_list_len, index_slot) = { + let index = accounts.accounts_index.read().unwrap(); + let account_entry = index.account_maps.get(&pubkey).unwrap(); + let slot_list = account_entry.1.read().unwrap(); + (slot_list.len(), slot_list[0].0) + }; + assert_eq!(slot_list_len, 1); + // Zero lamport entry was not the one purged + assert_eq!(index_slot, zero_lamport_slot); + // The ref count should still be 2 because no slots were purged + assert_eq!(accounts.ref_count_for_pubkey(&pubkey), 2); + + // storage for slot 1 had 2 accounts, now has 1 after pubkey 1 + // was reclaimed + check_storage(&accounts, 1, 1); + // storage for slot 2 had 1 accounts, now has 1 check_storage(&accounts, 2, 1); } From 04edca920deb4ac501c6cd9cdd2ce39256ef16d2 Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Sun, 4 Oct 2020 19:34:55 -0700 Subject: [PATCH 5/6] Some PR comments --- runtime/src/accounts_db.rs | 47 ++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 7084485bed1291..7ea6159eff5713 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -138,6 +138,13 @@ impl AccountStorage { }) .map(|account| (account, slot)) } + + fn slot_store_count(&self, slot: Slot, store_id: AppendVecId) -> Option { + self.0 + .get(&slot) + .and_then(|slot_storages| slot_storages.get(&store_id)) + .map(|store| store.count_and_status.read().unwrap().0) + } } #[derive(Debug, Eq, PartialEq, Copy, Clone, Deserialize, Serialize, AbiExample, AbiEnumVisitor)] @@ -541,13 +548,15 @@ impl AccountsDB { ) } - // Reclaim older states of rooted non-zero lamport accounts as a general - // AccountsDB bloat mitigation and preprocess for better zero-lamport purging. + // Reclaim older states of rooted accounts for AccountsDB bloat mitigation fn clean_old_rooted_accounts( &self, purges_in_root: Vec, max_clean_root: Option, ) -> (PurgedAccountSlots, ReclaimedAccounts) { + if purges_in_root.is_empty() { + return (HashMap::new(), HashMap::new()); + } // This number isn't carefully chosen; just guessed randomly such that // the hot loop will be the order of ~Xms. const INDEX_CLEAN_BULK_COUNT: usize = 4096; @@ -736,13 +745,8 @@ impl AccountsDB { accounts_scan.stop(); let mut clean_old_rooted = Measure::start("clean_old_roots"); - let (purged_account_slots, removed_accounts) = { - if !purges_in_root.is_empty() { - self.clean_old_rooted_accounts(purges_in_root, max_clean_root) - } else { - (HashMap::new(), HashMap::new()) - } - }; + let (purged_account_slots, removed_accounts) = + self.clean_old_rooted_accounts(purges_in_root, max_clean_root); self.do_reset_uncleaned_roots(&mut candidates, max_clean_root); clean_old_rooted.stop(); @@ -784,13 +788,13 @@ impl AccountsDB { } else { let mut key_set = HashSet::new(); key_set.insert(*key); - let count: usize = { - let storage = self.storage.read().unwrap(); - let slot_storage = storage.0.get(&slot).unwrap(); - let store = slot_storage.get(&account_info.store_id).unwrap(); - let count = store.count_and_status.read().unwrap().0; - count - 1 - }; + let count = self + .storage + .read() + .unwrap() + .slot_store_count(*slot, account_info.store_id) + .unwrap() + - 1; debug!( "store_counts, inserting slot: {}, store id: {}, count: {}", slot, account_info.store_id, count @@ -899,17 +903,16 @@ impl AccountsDB { assert!(dead_slots.contains(&expected_single_dead_slot)); } } - let purged_account_slots = if !dead_slots.is_empty() { - self.process_dead_slots(&dead_slots) - } else { - HashMap::new() - }; + let purged_account_slots = self.process_dead_slots(&dead_slots); (purged_account_slots, removed_accounts) } // Must be kept private!, does sensitive cleanup that should only be called from // supported pipelines in AccountsDb fn process_dead_slots(&self, dead_slots: &HashSet) -> PurgedAccountSlots { + if dead_slots.is_empty() { + return HashMap::new(); + } let mut clean_dead_slots = Measure::start("reclaims::purge_slots"); let purged_account_slots = self.clean_dead_slots(&dead_slots); clean_dead_slots.stop(); @@ -3444,7 +3447,7 @@ pub mod tests { let account = Account::new(0, 0, &Account::default().owner); accounts.store(latest_slot, &[(&pubkeys[30], &account)]); - // Create 10 new accounts in slot 1, should now have 11 = 10 = 21 + // Create 10 new accounts in slot 1, should now have 11 + 10 = 21 // accounts create_account(&accounts, &mut pubkeys1, latest_slot, 10, 0, 0); From 69092648864a17e4a21d5699a51b5c2b6b83c16e Mon Sep 17 00:00:00 2001 From: Carl Lin Date: Tue, 6 Oct 2020 15:31:55 -0700 Subject: [PATCH 6/6] Refactor and rename types --- runtime/src/accounts_db.rs | 80 +++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 32 deletions(-) diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 7ea6159eff5713..e701b2c75b56ec 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -100,8 +100,9 @@ pub type SnapshotStorages = Vec; // Each slot has a set of storage entries. pub(crate) type SlotStores = HashMap>; -type PurgedAccountSlots = HashMap>; -type ReclaimedAccounts = HashMap>; +type AccountSlots = HashMap>; +type AppendVecOffsets = HashMap>; +type ReclaimResult = (AccountSlots, AppendVecOffsets); trait Versioned { fn version(&self) -> u64; @@ -553,7 +554,7 @@ impl AccountsDB { &self, purges_in_root: Vec, max_clean_root: Option, - ) -> (PurgedAccountSlots, ReclaimedAccounts) { + ) -> ReclaimResult { if purges_in_root.is_empty() { return (HashMap::new(), HashMap::new()); } @@ -578,11 +579,12 @@ impl AccountsDB { inc_new_counter_info!("clean-old-root-par-clean-ms", clean_rooted.as_ms() as usize); let mut measure = Measure::start("clean_old_root_reclaims"); - let (purged_account_slots, removed_accounts) = self.handle_reclaims(&reclaims, None, false); + let mut reclaim_result = (HashMap::new(), HashMap::new()); + self.handle_reclaims(&reclaims, None, false, Some(&mut reclaim_result)); measure.stop(); debug!("{} {}", clean_rooted, measure); inc_new_counter_info!("clean-old-root-reclaim-ms", measure.as_ms() as usize); - (purged_account_slots, removed_accounts) + reclaim_result } fn do_reset_uncleaned_roots( @@ -839,7 +841,7 @@ impl AccountsDB { self.handle_dead_keys(dead_keys); - self.handle_reclaims(&reclaims, None, false); + self.handle_reclaims(&reclaims, None, false, None); reclaims_time.stop(); datapoint_info!( @@ -888,13 +890,19 @@ impl AccountsDB { reclaims: SlotSlice, expected_single_dead_slot: Option, no_dead_slot: bool, - ) -> (PurgedAccountSlots, ReclaimedAccounts) { + reclaim_result: Option<&mut ReclaimResult>, + ) { if reclaims.is_empty() { - return (HashMap::new(), HashMap::new()); + return; } - - let (dead_slots, removed_accounts) = - self.remove_dead_accounts(reclaims, expected_single_dead_slot); + let (purged_account_slots, reclaimed_offsets) = + if let Some((ref mut x, ref mut y)) = reclaim_result { + (Some(x), Some(y)) + } else { + (None, None) + }; + let dead_slots = + self.remove_dead_accounts(reclaims, expected_single_dead_slot, reclaimed_offsets); if no_dead_slot { assert!(dead_slots.is_empty()); } else if let Some(expected_single_dead_slot) = expected_single_dead_slot { @@ -903,18 +911,21 @@ impl AccountsDB { assert!(dead_slots.contains(&expected_single_dead_slot)); } } - let purged_account_slots = self.process_dead_slots(&dead_slots); - (purged_account_slots, removed_accounts) + self.process_dead_slots(&dead_slots, purged_account_slots); } // Must be kept private!, does sensitive cleanup that should only be called from // supported pipelines in AccountsDb - fn process_dead_slots(&self, dead_slots: &HashSet) -> PurgedAccountSlots { + fn process_dead_slots( + &self, + dead_slots: &HashSet, + purged_account_slots: Option<&mut AccountSlots>, + ) { if dead_slots.is_empty() { - return HashMap::new(); + return; } let mut clean_dead_slots = Measure::start("reclaims::purge_slots"); - let purged_account_slots = self.clean_dead_slots(&dead_slots); + self.clean_dead_slots(&dead_slots, purged_account_slots); clean_dead_slots.stop(); let mut purge_slots = Measure::start("reclaims::purge_slots"); @@ -928,7 +939,6 @@ impl AccountsDB { purge_slots, dead_slots, ); - purged_account_slots } fn do_shrink_stale_slot(&self, slot: Slot) -> usize { @@ -1090,7 +1100,7 @@ impl AccountsDB { update_index_elapsed = start.as_us(); let mut start = Measure::start("update_index_elapsed"); - self.handle_reclaims(&reclaims, Some(slot), true); + self.handle_reclaims(&reclaims, Some(slot), true, None); start.stop(); handle_reclaims_elapsed = start.as_us(); @@ -1492,7 +1502,7 @@ impl AccountsDB { // 1) Remove old bank hash from self.bank_hashes // 2) Purge this slot's storage entries from self.storage - self.handle_reclaims(&reclaims, Some(remove_slot), false); + self.handle_reclaims(&reclaims, Some(remove_slot), false, None); assert!(self.storage.read().unwrap().0.get(&remove_slot).is_none()); } @@ -2116,15 +2126,17 @@ impl AccountsDB { &self, reclaims: SlotSlice, expected_slot: Option, - ) -> (HashSet, ReclaimedAccounts) { + mut reclaimed_offsets: Option<&mut AppendVecOffsets>, + ) -> HashSet { let storage = self.storage.read().unwrap(); let mut dead_slots = HashSet::new(); - let mut removed_accounts: ReclaimedAccounts = HashMap::new(); for (slot, account_info) in reclaims { - removed_accounts - .entry(account_info.store_id) - .or_default() - .insert(account_info.offset); + if let Some(ref mut reclaimed_offsets) = reclaimed_offsets { + reclaimed_offsets + .entry(account_info.store_id) + .or_default() + .insert(account_info.offset); + } if let Some(expected_slot) = expected_slot { assert_eq!(*slot, expected_slot); } @@ -2153,11 +2165,14 @@ impl AccountsDB { true }); - (dead_slots, removed_accounts) + dead_slots } - fn clean_dead_slots(&self, dead_slots: &HashSet) -> PurgedAccountSlots { - let mut purged_account_slots: PurgedAccountSlots = HashMap::new(); + fn clean_dead_slots( + &self, + dead_slots: &HashSet, + mut purged_account_slots: Option<&mut AccountSlots>, + ) { { let mut measure = Measure::start("clean_dead_slots-ms"); let storage = self.storage.read().unwrap(); @@ -2190,7 +2205,9 @@ impl AccountsDB { }; let index = self.accounts_index.read().unwrap(); for (slot, pubkey) in slot_pubkeys { - purged_account_slots.entry(pubkey).or_default().insert(slot); + if let Some(ref mut purged_account_slots) = purged_account_slots { + purged_account_slots.entry(pubkey).or_default().insert(slot); + } index.unref_from_storage(&pubkey); } drop(index); @@ -2208,7 +2225,6 @@ impl AccountsDB { bank_hashes.remove(slot); } } - purged_account_slots } fn hash_accounts( @@ -2309,7 +2325,7 @@ impl AccountsDB { // b)From 1) we know no other slots are included in the "reclaims" // // From 1) and 2) we guarantee passing Some(slot), true is safe - self.handle_reclaims(&reclaims, Some(slot), true); + self.handle_reclaims(&reclaims, Some(slot), true, None); } pub fn add_root(&self, slot: Slot) { @@ -4606,7 +4622,7 @@ pub mod tests { let accounts = AccountsDB::new_single(); let mut dead_slots = HashSet::new(); dead_slots.insert(10); - accounts.clean_dead_slots(&dead_slots); + accounts.clean_dead_slots(&dead_slots, None); } #[test]