From 1fbd818647e7e87b9c706ddb26a28572d19a44c8 Mon Sep 17 00:00:00 2001 From: Trent Nelson Date: Fri, 21 Oct 2022 16:53:06 -0700 Subject: [PATCH] runtime: remove `Default` req on account scan interfaces (#28533) --- runtime/src/accounts.rs | 101 +++++++++++++++++--------------- runtime/src/accounts_db.rs | 114 +++++++++++++++++-------------------- 2 files changed, 109 insertions(+), 106 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index df1c73721e..b7e9576128 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -742,10 +742,11 @@ impl Accounts { if num == 0 { return Ok(vec![]); } - let account_balances = self.accounts_db.scan_accounts( + let mut account_balances = BinaryHeap::new(); + self.accounts_db.scan_accounts( ancestors, bank_id, - |collector: &mut BinaryHeap>, option| { + |option| { if let Some((pubkey, account, _slot)) = option { if account.lamports() == 0 { return; @@ -758,16 +759,16 @@ impl Accounts { if !collect { return; } - if collector.len() == num { - let Reverse(entry) = collector + if account_balances.len() == num { + let Reverse(entry) = account_balances .peek() .expect("BinaryHeap::peek should succeed when len > 0"); if *entry >= (account.lamports(), *pubkey) { return; } - collector.pop(); + account_balances.pop(); } - collector.push(Reverse((account.lamports(), *pubkey))); + account_balances.push(Reverse((account.lamports(), *pubkey))); } }, &ScanConfig::default(), @@ -878,16 +879,19 @@ impl Accounts { program_id: &Pubkey, config: &ScanConfig, ) -> ScanResult> { - self.accounts_db.scan_accounts( - ancestors, - bank_id, - |collector: &mut Vec, some_account_tuple| { - Self::load_while_filtering(collector, some_account_tuple, |account| { - account.owner() == program_id - }) - }, - config, - ) + let mut collector = Vec::new(); + self.accounts_db + .scan_accounts( + ancestors, + bank_id, + |some_account_tuple| { + Self::load_while_filtering(&mut collector, some_account_tuple, |account| { + account.owner() == program_id + }) + }, + config, + ) + .map(|_| collector) } pub fn load_by_program_with_filter bool>( @@ -898,16 +902,19 @@ impl Accounts { filter: F, config: &ScanConfig, ) -> ScanResult> { - self.accounts_db.scan_accounts( - ancestors, - bank_id, - |collector: &mut Vec, some_account_tuple| { - Self::load_while_filtering(collector, some_account_tuple, |account| { - account.owner() == program_id && filter(account) - }) - }, - config, - ) + let mut collector = Vec::new(); + self.accounts_db + .scan_accounts( + ancestors, + bank_id, + |some_account_tuple| { + Self::load_while_filtering(&mut collector, some_account_tuple, |account| { + account.owner() == program_id && filter(account) + }) + }, + config, + ) + .map(|_| collector) } fn calc_scan_result_size(account: &AccountSharedData) -> usize { @@ -957,14 +964,15 @@ impl Accounts { ) -> ScanResult> { let sum = AtomicUsize::default(); let config = config.recreate_with_abort(); + let mut collector = Vec::new(); let result = self .accounts_db .index_scan_accounts( ancestors, bank_id, *index_key, - |collector: &mut Vec, some_account_tuple| { - Self::load_while_filtering(collector, some_account_tuple, |account| { + |some_account_tuple| { + Self::load_while_filtering(&mut collector, some_account_tuple, |account| { let use_account = filter(account); if use_account && Self::accumulate_and_check_scan_result_size( @@ -981,7 +989,7 @@ impl Accounts { }, &config, ) - .map(|result| result.0); + .map(|_| collector); Self::maybe_abort_scan(result, &config) } @@ -994,18 +1002,21 @@ impl Accounts { ancestors: &Ancestors, bank_id: BankId, ) -> ScanResult> { - self.accounts_db.scan_accounts( - ancestors, - bank_id, - |collector: &mut Vec, some_account_tuple| { - if let Some((pubkey, account, slot)) = some_account_tuple - .filter(|(_, account, _)| Self::is_loadable(account.lamports())) - { - collector.push((*pubkey, account, slot)) - } - }, - &ScanConfig::default(), - ) + let mut collector = Vec::new(); + self.accounts_db + .scan_accounts( + ancestors, + bank_id, + |some_account_tuple| { + if let Some((pubkey, account, slot)) = some_account_tuple + .filter(|(_, account, _)| Self::is_loadable(account.lamports())) + { + collector.push((*pubkey, account, slot)) + } + }, + &ScanConfig::default(), + ) + .map(|_| collector) } pub fn hold_range_in_memory( @@ -1026,15 +1037,15 @@ impl Accounts { ancestors: &Ancestors, range: R, ) -> Vec { + let mut collector = Vec::new(); self.accounts_db.range_scan_accounts( "", // disable logging of this. We now parallelize it and this results in multiple parallel logs ancestors, range, &ScanConfig::new(true), - |collector: &mut Vec, option| { - Self::load_with_slot(collector, option) - }, - ) + |option| Self::load_with_slot(&mut collector, option), + ); + collector } /// Slow because lock is held for 1 operation instead of many. diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 795d514efd..c6d05e3734 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -4607,19 +4607,16 @@ impl AccountsDb { } } - pub fn scan_accounts( + pub fn scan_accounts( &self, ancestors: &Ancestors, bank_id: BankId, - scan_func: F, + mut scan_func: F, config: &ScanConfig, - ) -> ScanResult + ) -> ScanResult<()> where - F: Fn(&mut A, Option<(&Pubkey, AccountSharedData, Slot)>), - A: Default, + F: FnMut(Option<(&Pubkey, AccountSharedData, Slot)>), { - let mut collector = A::default(); - // This can error out if the slots being scanned over are aborted self.accounts_index.scan_accounts( ancestors, @@ -4629,26 +4626,23 @@ impl AccountsDb { .get_account_accessor(slot, pubkey, &account_info.storage_location()) .get_loaded_account() .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)); - scan_func(&mut collector, account_slot) + scan_func(account_slot) }, config, )?; - Ok(collector) + Ok(()) } - pub fn unchecked_scan_accounts( + pub fn unchecked_scan_accounts( &self, metric_name: &'static str, ancestors: &Ancestors, - scan_func: F, + mut scan_func: F, config: &ScanConfig, - ) -> A - where - F: Fn(&mut A, (&Pubkey, LoadedAccount, Slot)), - A: Default, + ) where + F: FnMut(&Pubkey, LoadedAccount, Slot), { - let mut collector = A::default(); self.accounts_index.unchecked_scan_accounts( metric_name, ancestors, @@ -4657,29 +4651,25 @@ impl AccountsDb { .get_account_accessor(slot, pubkey, &account_info.storage_location()) .get_loaded_account() { - scan_func(&mut collector, (pubkey, loaded_account, slot)); + scan_func(pubkey, loaded_account, slot); } }, config, ); - collector } /// Only guaranteed to be safe when called from rent collection - pub fn range_scan_accounts( + pub fn range_scan_accounts( &self, metric_name: &'static str, ancestors: &Ancestors, range: R, config: &ScanConfig, - scan_func: F, - ) -> A - where - F: Fn(&mut A, Option<(&Pubkey, AccountSharedData, Slot)>), - A: Default, + mut scan_func: F, + ) where + F: FnMut(Option<(&Pubkey, AccountSharedData, Slot)>), R: RangeBounds + std::fmt::Debug, { - let mut collector = A::default(); self.accounts_index.range_scan_accounts( metric_name, ancestors, @@ -4700,24 +4690,22 @@ impl AccountsDb { .get_loaded_account() .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)) { - scan_func(&mut collector, Some(account_slot)) + scan_func(Some(account_slot)) } }, ); - collector } - pub fn index_scan_accounts( + pub fn index_scan_accounts( &self, ancestors: &Ancestors, bank_id: BankId, index_key: IndexKey, - scan_func: F, + mut scan_func: F, config: &ScanConfig, - ) -> ScanResult<(A, bool)> + ) -> ScanResult where - F: Fn(&mut A, Option<(&Pubkey, AccountSharedData, Slot)>), - A: Default, + F: FnMut(Option<(&Pubkey, AccountSharedData, Slot)>), { let key = match &index_key { IndexKey::ProgramId(key) => key, @@ -4727,11 +4715,10 @@ impl AccountsDb { if !self.account_indexes.include_key(key) { // the requested key was not indexed in the secondary index, so do a normal scan let used_index = false; - let scan_result = self.scan_accounts(ancestors, bank_id, scan_func, config)?; - return Ok((scan_result, used_index)); + self.scan_accounts(ancestors, bank_id, scan_func, config)?; + return Ok(used_index); } - let mut collector = A::default(); self.accounts_index.index_scan_accounts( ancestors, bank_id, @@ -4741,12 +4728,12 @@ impl AccountsDb { .get_account_accessor(slot, pubkey, &account_info.storage_location()) .get_loaded_account() .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)); - scan_func(&mut collector, account_slot) + scan_func(account_slot) }, config, )?; let used_index = true; - Ok((collector, used_index)) + Ok(used_index) } /// Scan a specific slot through all the account storage in parallel @@ -10551,11 +10538,12 @@ pub mod tests { &account1 ); - let accounts: Vec = db.unchecked_scan_accounts( + let mut accounts = Vec::new(); + db.unchecked_scan_accounts( "", &ancestors, - |accounts: &mut Vec, option| { - accounts.push(option.1.take_account()); + |_, account, _| { + accounts.push(account.take_account()); }, &ScanConfig::default(), ); @@ -11482,40 +11470,42 @@ pub mod tests { keys: [mint_key].iter().cloned().collect::>(), }); // Secondary index can't be used - do normal scan: should still find both pubkeys - let found_accounts = accounts + let mut found_accounts = HashSet::new(); + let used_index = accounts .index_scan_accounts( &Ancestors::default(), bank_id, index_key, - |collection: &mut HashSet, account| { - collection.insert(*account.unwrap().0); + |account| { + found_accounts.insert(*account.unwrap().0); }, &ScanConfig::default(), ) .unwrap(); - assert!(!found_accounts.1); - assert_eq!(found_accounts.0.len(), 2); - assert!(found_accounts.0.contains(&pubkey1)); - assert!(found_accounts.0.contains(&pubkey2)); + assert!(!used_index); + assert_eq!(found_accounts.len(), 2); + assert!(found_accounts.contains(&pubkey1)); + assert!(found_accounts.contains(&pubkey2)); accounts.account_indexes.keys = None; // Secondary index can now be used since it isn't marked as excluded - let found_accounts = accounts + let mut found_accounts = HashSet::new(); + let used_index = accounts .index_scan_accounts( &Ancestors::default(), bank_id, index_key, - |collection: &mut HashSet, account| { - collection.insert(*account.unwrap().0); + |account| { + found_accounts.insert(*account.unwrap().0); }, &ScanConfig::default(), ) .unwrap(); - assert!(found_accounts.1); - assert_eq!(found_accounts.0.len(), 2); - assert!(found_accounts.0.contains(&pubkey1)); - assert!(found_accounts.0.contains(&pubkey2)); + assert!(used_index); + assert_eq!(found_accounts.len(), 2); + assert!(found_accounts.contains(&pubkey1)); + assert!(found_accounts.contains(&pubkey2)); accounts.account_indexes.keys = None; } @@ -12124,22 +12114,24 @@ pub mod tests { db.store_uncached(1, &[(&key1, &account1)]); let ancestors = vec![(0, 0)].into_iter().collect(); - let accounts: Vec = db.unchecked_scan_accounts( + let mut accounts = Vec::new(); + db.unchecked_scan_accounts( "", &ancestors, - |accounts: &mut Vec, option| { - accounts.push(option.1.take_account()); + |_, account, _| { + accounts.push(account.take_account()); }, &ScanConfig::default(), ); assert_eq!(accounts, vec![account0]); let ancestors = vec![(1, 1), (0, 0)].into_iter().collect(); - let accounts: Vec = db.unchecked_scan_accounts( + let mut accounts = Vec::new(); + db.unchecked_scan_accounts( "", &ancestors, - |accounts: &mut Vec, option| { - accounts.push(option.1.take_account()); + |_, account, _| { + accounts.push(account.take_account()); }, &ScanConfig::default(), ); @@ -14389,7 +14381,7 @@ pub mod tests { db.scan_accounts( &scan_ancestors, bank_id, - |_collector: &mut Vec<(Pubkey, AccountSharedData)>, maybe_account| { + |maybe_account| { ready_.store(true, Ordering::Relaxed); if let Some((pubkey, _, _)) = maybe_account { if *pubkey == stall_key {