From c2ce2c8f7b66ed430761eb576395405fd138a774 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 | 81 ++++++++---------- runtime/src/accounts_db.rs | 169 +++++++++++++++---------------------- 2 files changed, 104 insertions(+), 146 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index ddff6fe7ec02f0..fab8a3ab76863b 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -614,10 +614,9 @@ impl Accounts { if num == 0 { return Ok(vec![]); } - let account_balances = self.accounts_db.scan_accounts( - ancestors, - bank_id, - |collector: &mut BinaryHeap>, option| { + let mut account_balances = BinaryHeap::new(); + self.accounts_db + .scan_accounts(ancestors, bank_id, |option| { if let Some((pubkey, account, _slot)) = option { if account.lamports() == 0 { return; @@ -630,19 +629,18 @@ 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))); } - }, - )?; + })?; Ok(account_balances .into_sorted_vec() .into_iter() @@ -717,15 +715,14 @@ impl Accounts { bank_id: BankId, program_id: &Pubkey, ) -> ScanResult> { - self.accounts_db.scan_accounts( - ancestors, - bank_id, - |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { - Self::load_while_filtering(collector, some_account_tuple, |account| { + 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 }) - }, - ) + }) + .map(|_| collector) } pub fn load_by_program_with_filter bool>( @@ -735,15 +732,14 @@ impl Accounts { program_id: &Pubkey, filter: F, ) -> ScanResult> { - self.accounts_db.scan_accounts( - ancestors, - bank_id, - |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { - Self::load_while_filtering(collector, some_account_tuple, |account| { + 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) }) - }, - ) + }) + .map(|_| collector) } pub fn load_by_index_key_with_filter bool>( @@ -753,18 +749,14 @@ impl Accounts { index_key: &IndexKey, filter: F, ) -> ScanResult> { + let mut collector = Vec::new(); self.accounts_db - .index_scan_accounts( - ancestors, - bank_id, - *index_key, - |collector: &mut Vec<(Pubkey, AccountSharedData)>, some_account_tuple| { - Self::load_while_filtering(collector, some_account_tuple, |account| { - filter(account) - }) - }, - ) - .map(|result| result.0) + .index_scan_accounts(ancestors, bank_id, *index_key, |some_account_tuple| { + Self::load_while_filtering(&mut collector, some_account_tuple, |account| { + filter(account) + }) + }) + .map(|_| collector) } pub fn account_indexes_include_key(&self, key: &Pubkey) -> bool { @@ -776,17 +768,16 @@ impl Accounts { ancestors: &Ancestors, bank_id: BankId, ) -> ScanResult> { - self.accounts_db.scan_accounts( - ancestors, - bank_id, - |collector: &mut Vec<(Pubkey, AccountSharedData, Slot)>, some_account_tuple| { + 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)) } - }, - ) + }) + .map(|_| collector) } pub fn load_to_collect_rent_eagerly>( @@ -794,14 +785,14 @@ impl Accounts { ancestors: &Ancestors, range: R, ) -> Vec<(Pubkey, AccountSharedData)> { + let mut collector = Vec::new(); self.accounts_db.range_scan_accounts( "load_to_collect_rent_eagerly_scan_elapsed", ancestors, range, - |collector: &mut Vec<(Pubkey, AccountSharedData)>, option| { - Self::load_while_filtering(collector, option, |_| true) - }, - ) + |option| Self::load_while_filtering(&mut collector, option, |_| true), + ); + 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 e9bab4b66d2c2a..00a9aa44c70cb8 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -2506,18 +2506,15 @@ impl AccountsDb { } } - pub fn scan_accounts( + pub fn scan_accounts( &self, ancestors: &Ancestors, bank_id: BankId, - scan_func: F, - ) -> ScanResult + mut scan_func: F, + ) -> 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, bank_id, |pubkey, (account_info, slot)| { @@ -2525,23 +2522,20 @@ impl AccountsDb { .get_account_accessor(slot, pubkey, account_info.store_id, account_info.offset) .get_loaded_account() .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)); - scan_func(&mut collector, account_slot) + scan_func(account_slot) })?; - Ok(collector) + Ok(()) } - pub fn unchecked_scan_accounts( + pub fn unchecked_scan_accounts( &self, metric_name: &'static str, ancestors: &Ancestors, - scan_func: F, - ) -> A - where - F: Fn(&mut A, (&Pubkey, LoadedAccount, Slot)), - A: Default, + mut scan_func: F, + ) where + F: FnMut(&Pubkey, LoadedAccount, Slot), { - let mut collector = A::default(); self.accounts_index.unchecked_scan_accounts( metric_name, ancestors, @@ -2550,26 +2544,22 @@ impl AccountsDb { .get_account_accessor(slot, pubkey, account_info.store_id, account_info.offset) .get_loaded_account() { - scan_func(&mut collector, (pubkey, loaded_account, slot)); + scan_func(pubkey, loaded_account, slot); } }, ); - collector } - pub fn range_scan_accounts( + pub fn range_scan_accounts( &self, metric_name: &'static str, ancestors: &Ancestors, range: R, - 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, { - let mut collector = A::default(); self.accounts_index.range_scan_accounts( metric_name, ancestors, @@ -2584,27 +2574,26 @@ impl AccountsDb { // meaning no other subsystems can invalidate the account_info before making their // changes to the index entry. // For details, see the comment in retry_to_get_account_accessor() - let account_slot = self + if let Some(account_slot) = self .get_account_accessor(slot, pubkey, account_info.store_id, account_info.offset) .get_loaded_account() .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)) - .unwrap(); - 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, - ) -> ScanResult<(A, bool)> + mut scan_func: F, + ) -> 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, @@ -2614,11 +2603,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)?; - return Ok((scan_result, used_index)); + self.scan_accounts(ancestors, bank_id, scan_func)?; + return Ok(used_index); } - let mut collector = A::default(); self.accounts_index.index_scan_accounts( ancestors, bank_id, @@ -2628,11 +2616,11 @@ impl AccountsDb { .get_account_accessor(slot, pubkey, account_info.store_id, account_info.offset) .get_loaded_account() .map(|loaded_account| (pubkey, loaded_account.take_account(), slot)); - scan_func(&mut collector, account_slot) + scan_func(account_slot) }, )?; let used_index = true; - Ok((collector, used_index)) + Ok(used_index) } /// Scan a specific slot through all the account storage in parallel @@ -6932,13 +6920,10 @@ pub mod tests { &account1 ); - let accounts: Vec = db.unchecked_scan_accounts( - "", - &ancestors, - |accounts: &mut Vec, option| { - accounts.push(option.1.take_account()); - }, - ); + let mut accounts = Vec::new(); + db.unchecked_scan_accounts("", &ancestors, |_, account, _| { + accounts.push(account.take_account()); + }); assert_eq!(accounts, vec![account1]); } @@ -7855,38 +7840,30 @@ 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 - .index_scan_accounts( - &Ancestors::default(), - bank_id, - index_key, - |collection: &mut HashSet, account| { - collection.insert(*account.unwrap().0); - }, - ) + let mut found_accounts = HashSet::new(); + let used_index = accounts + .index_scan_accounts(&Ancestors::default(), bank_id, index_key, |account| { + found_accounts.insert(*account.unwrap().0); + }) .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 - .index_scan_accounts( - &Ancestors::default(), - bank_id, - index_key, - |collection: &mut HashSet, account| { - collection.insert(*account.unwrap().0); - }, - ) + let mut found_accounts = HashSet::new(); + let used_index = accounts + .index_scan_accounts(&Ancestors::default(), bank_id, index_key, |account| { + found_accounts.insert(*account.unwrap().0); + }) .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; } @@ -8468,23 +8445,17 @@ pub mod tests { db.store_uncached(1, &[(&key1, &account1)]); let ancestors = vec![(0, 0)].into_iter().collect(); - let accounts: Vec = db.unchecked_scan_accounts( - "", - &ancestors, - |accounts: &mut Vec, option| { - accounts.push(option.1.take_account()); - }, - ); + let mut accounts = Vec::new(); + db.unchecked_scan_accounts("", &ancestors, |_, account, _| { + accounts.push(account.take_account()); + }); assert_eq!(accounts, vec![account0]); let ancestors = vec![(1, 1), (0, 0)].into_iter().collect(); - let accounts: Vec = db.unchecked_scan_accounts( - "", - &ancestors, - |accounts: &mut Vec, option| { - accounts.push(option.1.take_account()); - }, - ); + let mut accounts = Vec::new(); + db.unchecked_scan_accounts("", &ancestors, |_, account, _| { + accounts.push(account.take_account()); + }); assert_eq!(accounts.len(), 2); } @@ -10454,24 +10425,20 @@ pub mod tests { let t_scan = Builder::new() .name("scan".to_string()) .spawn(move || { - db.scan_accounts( - &scan_ancestors, - bank_id, - |_collector: &mut Vec<(Pubkey, AccountSharedData)>, maybe_account| { - ready_.store(true, Ordering::Relaxed); - if let Some((pubkey, _, _)) = maybe_account { - if *pubkey == stall_key { - loop { - if exit_.load(Ordering::Relaxed) { - break; - } else { - sleep(Duration::from_millis(10)); - } + db.scan_accounts(&scan_ancestors, bank_id, |maybe_account| { + ready_.store(true, Ordering::Relaxed); + if let Some((pubkey, _, _)) = maybe_account { + if *pubkey == stall_key { + loop { + if exit_.load(Ordering::Relaxed) { + break; + } else { + sleep(Duration::from_millis(10)); } } } - }, - ) + } + }) .unwrap(); }) .unwrap();