From 3d409155c18752efc818fd2ab50d82e13f8dcfdd 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 | 47 +++++++++------ runtime/src/accounts_db.rs | 119 +++++++++++++++++-------------------- 2 files changed, 85 insertions(+), 81 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index ddff6fe7ec02f0..2369dfb8b691f3 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -614,10 +614,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; @@ -630,16 +631,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))); } }, )?; @@ -717,15 +718,17 @@ impl Accounts { bank_id: BankId, program_id: &Pubkey, ) -> ScanResult> { + let mut collector = Vec::new(); 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| { + |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 +738,17 @@ impl Accounts { program_id: &Pubkey, filter: F, ) -> ScanResult> { + let mut collector = Vec::new(); 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| { + |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 +758,20 @@ impl Accounts { index_key: &IndexKey, filter: F, ) -> ScanResult> { - self.accounts_db + 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| { + |some_account_tuple| { + Self::load_while_filtering(&mut collector, some_account_tuple, |account| { filter(account) }) }, ) - .map(|result| result.0) + .map(|_| collector) } pub fn account_indexes_include_key(&self, key: &Pubkey) -> bool { @@ -776,10 +783,11 @@ impl Accounts { ancestors: &Ancestors, bank_id: BankId, ) -> ScanResult> { + let mut collector = Vec::new(); self.accounts_db.scan_accounts( ancestors, bank_id, - |collector: &mut Vec<(Pubkey, AccountSharedData, Slot)>, some_account_tuple| { + |some_account_tuple| { if let Some((pubkey, account, slot)) = some_account_tuple .filter(|(_, account, _)| Self::is_loadable(account.lamports())) { @@ -787,6 +795,7 @@ impl Accounts { } }, ) + .map(|_| collector) } pub fn load_to_collect_rent_eagerly>( @@ -794,14 +803,16 @@ 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..57d83ec464bf84 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,11 +6920,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()); }, ); assert_eq!(accounts, vec![account1]); @@ -7855,38 +7844,40 @@ 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); }, ) .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); }, ) .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,21 +8459,23 @@ 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()); }, ); 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()); }, ); assert_eq!(accounts.len(), 2); @@ -10457,7 +10450,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 {