From c09cbbb778a9491cec34f77920a8096a4de496af Mon Sep 17 00:00:00 2001 From: "Jeff Washington (jwash)" Date: Tue, 17 Oct 2023 14:49:46 -0700 Subject: [PATCH] sort ancient append vec target_slots_sorted (#33729) --- accounts-db/src/ancient_append_vecs.rs | 328 ++++++++++++++----------- 1 file changed, 182 insertions(+), 146 deletions(-) diff --git a/accounts-db/src/ancient_append_vecs.rs b/accounts-db/src/ancient_append_vecs.rs index 46e299d3eefac3..c4ba88c3cc6434 100644 --- a/accounts-db/src/ancient_append_vecs.rs +++ b/accounts-db/src/ancient_append_vecs.rs @@ -586,6 +586,7 @@ impl AccountsDb { remove.into_iter().rev().for_each(|i| { accounts_to_combine.remove(i); }); + target_slots_sorted.sort_unstable(); AccountsToCombine { accounts_to_combine, accounts_keep_slots, @@ -717,6 +718,7 @@ struct AccountsToCombine<'a> { /// these slots will NOT be in 'accounts_keep_slots' /// Some of these slots will have ancient append vecs created at them to contain everything in 'accounts_to_combine' /// The rest will become dead slots with no accounts in them. + /// Sort order is lowest to highest. target_slots_sorted: Vec, /// when scanning, this many slots contained accounts that could not be packed because accounts with ref_count > 1 existed. unpackable_slots_count: usize, @@ -1419,167 +1421,201 @@ pub mod tests { for add_dead_account in [true, false] { for method in TestWriteMultipleRefs::iter() { for num_slots in 0..3 { - for two_refs in [false, true] { - let (db, storages, slots, infos) = get_sample_storages(num_slots, None); - let original_results = storages - .iter() - .map(|store| db.get_unique_accounts_from_storage(store)) - .collect::>(); - if two_refs { - original_results.iter().for_each(|results| { - results.stored_accounts.iter().for_each(|account| { - let entry = db - .accounts_index - .get_account_read_entry(account.pubkey()) - .unwrap(); - entry.addref(); - }) - }); - } + for unsorted_slots in [false, true] { + for two_refs in [false, true] { + let (db, mut storages, slots, mut infos) = + get_sample_storages(num_slots, None); + let slots_vec; + if unsorted_slots { + slots_vec = slots.rev().collect::>(); + storages = storages.into_iter().rev().collect(); + infos = infos.into_iter().rev().collect(); + } else { + slots_vec = slots.collect::>() + } - if add_dead_account { - storages.iter().for_each(|storage| { - let pk = solana_sdk::pubkey::new_rand(); - let alive = false; - let write_version = 0; - append_single_account_with_default_hash( - storage, - &pk, - &AccountSharedData::default(), - write_version, - alive, - Some(&db.accounts_index), - ); - assert!(db.accounts_index.purge_exact( - &pk, - &[storage.slot()] - .into_iter() - .collect::>(), - &mut Vec::default() - )); - }); - } - let original_results = storages - .iter() - .map(|store| db.get_unique_accounts_from_storage(store)) - .collect::>(); + let original_results = storages + .iter() + .map(|store| db.get_unique_accounts_from_storage(store)) + .collect::>(); + if two_refs { + original_results.iter().for_each(|results| { + results.stored_accounts.iter().for_each(|account| { + let entry = db + .accounts_index + .get_account_read_entry(account.pubkey()) + .unwrap(); + entry.addref(); + }) + }); + } - let accounts_per_storage = infos - .iter() - .zip(original_results.into_iter()) - .collect::>(); + if add_dead_account { + storages.iter().for_each(|storage| { + let pk = solana_sdk::pubkey::new_rand(); + let alive = false; + let write_version = 0; + append_single_account_with_default_hash( + storage, + &pk, + &AccountSharedData::default(), + write_version, + alive, + Some(&db.accounts_index), + ); + assert!(db.accounts_index.purge_exact( + &pk, + &[storage.slot()] + .into_iter() + .collect::>(), + &mut Vec::default() + )); + }); + } + let original_results = storages + .iter() + .map(|store| db.get_unique_accounts_from_storage(store)) + .collect::>(); - let accounts_to_combine = - db.calc_accounts_to_combine(&accounts_per_storage); - let slots_vec = slots.collect::>(); - if !add_dead_account && two_refs { - assert!(accounts_to_combine.accounts_to_combine.is_empty()); - continue; - } else { - assert_eq!( + let accounts_per_storage = infos + .iter() + .zip(original_results.into_iter()) + .collect::>(); + + let accounts_to_combine = + db.calc_accounts_to_combine(&accounts_per_storage); + if !add_dead_account && two_refs { + assert!(accounts_to_combine.accounts_to_combine.is_empty()); + continue; + } else { + assert_eq!( accounts_to_combine.accounts_to_combine.len(), num_slots, "method: {method:?}, num_slots: {num_slots}, two_refs: {two_refs}" ); - } - if two_refs { - // all accounts should be in many_refs - let mut accounts_keep = accounts_to_combine - .accounts_keep_slots - .keys() - .cloned() - .collect::>(); - assert!(!accounts_to_combine - .accounts_to_combine - .iter() - .any(|a| a.unrefed_pubkeys.is_empty())); - accounts_keep.sort_unstable(); - assert_eq!(accounts_keep, slots_vec); - assert!(accounts_to_combine.target_slots_sorted.is_empty()); - assert_eq!(accounts_to_combine.accounts_keep_slots.len(), num_slots); - assert!(accounts_to_combine.accounts_to_combine.iter().all( - |shrink_collect| shrink_collect - .alive_accounts - .one_ref - .accounts - .is_empty() - )); - assert!(accounts_to_combine.accounts_to_combine.iter().all( - |shrink_collect| shrink_collect - .alive_accounts - .many_refs - .accounts - .is_empty() - )); - } else { - if add_dead_account { + } + if two_refs { + // all accounts should be in many_refs + let mut accounts_keep = accounts_to_combine + .accounts_keep_slots + .keys() + .cloned() + .collect::>(); assert!(!accounts_to_combine .accounts_to_combine .iter() .any(|a| a.unrefed_pubkeys.is_empty())); + // sort because accounts_keep_slots is a hashmap, with non-deterministic ordering + accounts_keep.sort_unstable(); + if unsorted_slots { + accounts_keep = accounts_keep.into_iter().rev().collect(); + } + assert_eq!(accounts_keep, slots_vec); + assert!(accounts_to_combine.target_slots_sorted.is_empty()); + assert_eq!( + accounts_to_combine.accounts_keep_slots.len(), + num_slots + ); + assert!(accounts_to_combine.accounts_to_combine.iter().all( + |shrink_collect| shrink_collect + .alive_accounts + .one_ref + .accounts + .is_empty() + )); + assert!(accounts_to_combine.accounts_to_combine.iter().all( + |shrink_collect| shrink_collect + .alive_accounts + .many_refs + .accounts + .is_empty() + )); + } else { + if add_dead_account { + assert!(!accounts_to_combine + .accounts_to_combine + .iter() + .any(|a| a.unrefed_pubkeys.is_empty())); + } + // all accounts should be in one_ref and all slots are available as target slots + assert_eq!( + accounts_to_combine.target_slots_sorted, + if unsorted_slots { + slots_vec.iter().cloned().rev().collect::>() + } else { + slots_vec.clone() + } + ); + assert!(accounts_to_combine.accounts_keep_slots.is_empty()); + assert!(accounts_to_combine.accounts_to_combine.iter().all( + |shrink_collect| !shrink_collect + .alive_accounts + .one_ref + .accounts + .is_empty() + )); + assert!(accounts_to_combine.accounts_to_combine.iter().all( + |shrink_collect| shrink_collect + .alive_accounts + .many_refs + .accounts + .is_empty() + )); } - // all accounts should be in one_ref and all slots are available as target slots - assert_eq!(accounts_to_combine.target_slots_sorted, slots_vec); - assert!(accounts_to_combine.accounts_keep_slots.is_empty()); - assert!(accounts_to_combine.accounts_to_combine.iter().all( - |shrink_collect| !shrink_collect - .alive_accounts - .one_ref - .accounts - .is_empty() - )); - assert!(accounts_to_combine.accounts_to_combine.iter().all( - |shrink_collect| shrink_collect - .alive_accounts - .many_refs - .accounts - .is_empty() - )); - } - // test write_ancient_accounts_to_same_slot_multiple_refs since we built interesting 'AccountsToCombine' - let write_ancient_accounts = match method { - TestWriteMultipleRefs::MultipleRefs => { - let mut write_ancient_accounts = WriteAncientAccounts::default(); - db.write_ancient_accounts_to_same_slot_multiple_refs( - accounts_to_combine.accounts_keep_slots.values(), - &mut write_ancient_accounts, + // test write_ancient_accounts_to_same_slot_multiple_refs since we built interesting 'AccountsToCombine' + let write_ancient_accounts = match method { + TestWriteMultipleRefs::MultipleRefs => { + let mut write_ancient_accounts = + WriteAncientAccounts::default(); + db.write_ancient_accounts_to_same_slot_multiple_refs( + accounts_to_combine.accounts_keep_slots.values(), + &mut write_ancient_accounts, + ); + write_ancient_accounts + } + TestWriteMultipleRefs::PackedStorages => { + let packed_contents = Vec::default(); + db.write_packed_storages(&accounts_to_combine, packed_contents) + } + }; + if two_refs { + assert_eq!( + write_ancient_accounts.shrinks_in_progress.len(), + num_slots ); - write_ancient_accounts - } - TestWriteMultipleRefs::PackedStorages => { - let packed_contents = Vec::default(); - db.write_packed_storages(&accounts_to_combine, packed_contents) - } - }; - if two_refs { - assert_eq!(write_ancient_accounts.shrinks_in_progress.len(), num_slots); - let mut shrinks_in_progress = write_ancient_accounts - .shrinks_in_progress - .iter() - .collect::>(); - shrinks_in_progress.sort_unstable_by(|a, b| a.0.cmp(b.0)); - assert_eq!( - shrinks_in_progress - .iter() - .map(|(slot, _)| **slot) - .collect::>(), - slots_vec - ); - assert_eq!( - shrinks_in_progress + let mut shrinks_in_progress = write_ancient_accounts + .shrinks_in_progress .iter() - .map(|(_, shrink_in_progress)| shrink_in_progress - .old_storage() - .append_vec_id()) - .collect::>(), - storages - .iter() - .map(|storage| storage.append_vec_id()) - .collect::>() - ); - } else { - assert!(write_ancient_accounts.shrinks_in_progress.is_empty()); + .collect::>(); + // sort because shrinks_in_progress is a HashMap with non-deterministic order + shrinks_in_progress.sort_unstable_by(|a, b| a.0.cmp(b.0)); + if unsorted_slots { + shrinks_in_progress = + shrinks_in_progress.into_iter().rev().collect(); + } + assert_eq!( + shrinks_in_progress + .iter() + .map(|(slot, _)| **slot) + .collect::>(), + slots_vec + ); + assert_eq!( + shrinks_in_progress + .iter() + .map(|(_, shrink_in_progress)| shrink_in_progress + .old_storage() + .append_vec_id()) + .collect::>(), + storages + .iter() + .map(|storage| storage.append_vec_id()) + .collect::>() + ); + } else { + assert!(write_ancient_accounts.shrinks_in_progress.is_empty()); + } } } }