Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggressively shrink ancient storages when shrink isn't too busy. #2946

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open
108 changes: 107 additions & 1 deletion accounts-db/src/accounts_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,11 @@ const MAX_ITEMS_PER_CHUNK: Slot = 2_500;
// This allows us to split up accounts index accesses across multiple threads.
const SHRINK_COLLECT_CHUNK_SIZE: usize = 50;

/// The number of shrink candidate slots that is small enough so that
/// additional storages from ancient slots can be added to the
/// candidates for shrinking.
const SHRINK_INSERT_ANCIENT_THRESHOLD: usize = 10;

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
pub enum CreateAncientStorage {
/// ancient storages are created by appending
Expand Down Expand Up @@ -1463,6 +1468,11 @@ pub struct AccountsDb {
/// Flag to indicate if the experimental accounts lattice hash is enabled.
/// (For R&D only; a feature-gate also exists to turn this on and make it a part of consensus.)
pub is_experimental_accumulator_hash_enabled: AtomicBool,

/// These are the ancient storages that could be valuable to shrink.
/// sorted by largest dead bytes to smallest
/// Members are Slot and capacity. If capacity is smaller, then that means the storage was already shrunk.
pub(crate) best_ancient_slots_to_shrink: RwLock<Vec<(Slot, u64)>>,
}

/// results from 'split_storages_ancient'
Expand Down Expand Up @@ -1811,6 +1821,7 @@ impl AccountsDb {
is_experimental_accumulator_hash_enabled: default_accounts_db_config
.enable_experimental_accumulator_hash
.into(),
best_ancient_slots_to_shrink: RwLock::default(),
}
}

Expand Down Expand Up @@ -4352,8 +4363,12 @@ impl AccountsDb {

let shrink_candidates_slots =
std::mem::take(&mut *self.shrink_candidate_slots.lock().unwrap());
self.shrink_stats
jeffwashington marked this conversation as resolved.
Show resolved Hide resolved
.initial_candidates_count
.store(shrink_candidates_slots.len() as u64, Ordering::Relaxed);

let candidates_count = shrink_candidates_slots.len();
let ((shrink_slots, shrink_slots_next_batch), select_time_us) = measure_us!({
let ((mut shrink_slots, shrink_slots_next_batch), select_time_us) = measure_us!({
if let AccountShrinkThreshold::TotalSpace { shrink_ratio } = self.shrink_ratio {
let (shrink_slots, shrink_slots_next_batch) =
self.select_candidates_by_total_usage(&shrink_candidates_slots, shrink_ratio);
Expand All @@ -4374,6 +4389,30 @@ impl AccountsDb {
}
});

// If there are too few slots to shrink, add an ancient slot
// for shrinking.
if shrink_slots.len() < SHRINK_INSERT_ANCIENT_THRESHOLD {
let mut ancients = self.best_ancient_slots_to_shrink.write().unwrap();
ancients.reverse();
while let Some((slot, capacity)) = ancients.pop() {
if let Some(store) = self.storage.get_slot_storage_entry(slot) {
if !shrink_slots.contains(&slot)
&& capacity == store.capacity()
&& Self::is_candidate_for_shrink(self, &store)
{
let ancient_bytes_added_to_shrink = store.alive_bytes() as u64;
shrink_slots.insert(slot, store);
self.shrink_stats
.ancient_bytes_added_to_shrink
.fetch_add(ancient_bytes_added_to_shrink, Ordering::Relaxed);
self.shrink_stats
.ancient_slots_added_to_shrink
.fetch_add(1, Ordering::Relaxed);
break;
}
}
}
}
if shrink_slots.is_empty()
&& shrink_slots_next_batch
.as_ref()
Expand Down Expand Up @@ -12015,6 +12054,73 @@ pub mod tests {
);
}

/// This test creates an ancient storage with three alive accounts
/// of various sizes. It then simulates killing one of the
/// accounts in a more recent (non-ancient) slot by overwriting
/// the account that has the smallest data size. The dead account
/// is expected to be deleted from its ancient storage in the
/// process of shrinking candidate slots. The capacity of the
/// storage after shrinking is expected to be the sum of alive
/// bytes of the two remaining alive ancient accounts.
#[test]
fn test_shrink_candidate_slots_with_dead_ancient_account() {
solana_logger::setup();
let epoch_schedule = EpochSchedule::default();
let num_ancient_slots = 3;
// Prepare 3 append vecs to combine [medium, big, small]
let account_data_sizes = vec![1000, 2000, 150];
let (db, starting_ancient_slot) =
create_db_with_storages_and_index_with_customized_account_size_per_slot(
true,
num_ancient_slots,
account_data_sizes,
);
db.add_root(starting_ancient_slot);
let slots_to_combine: Vec<Slot> =
(starting_ancient_slot..starting_ancient_slot + num_ancient_slots as Slot).collect();
db.combine_ancient_slots(slots_to_combine, CAN_RANDOMLY_SHRINK_FALSE);
let storage = db.get_storage_for_slot(starting_ancient_slot).unwrap();
let ancient_accounts = db.get_unique_accounts_from_storage(&storage);
// Check that three accounts are indeed present in the combined storage.
assert_eq!(ancient_accounts.stored_accounts.len(), 3);
// Find an ancient account with smallest data length.
// This will be a dead account, overwritten in the current slot.
let modified_account_pubkey = ancient_accounts
.stored_accounts
.iter()
.min_by(|a, b| a.data_len.cmp(&b.data_len))
.unwrap()
.pubkey;
let modified_account_owner = *AccountSharedData::default().owner();
let modified_account = AccountSharedData::new(223, 0, &modified_account_owner);
let ancient_append_vec_offset = db.ancient_append_vec_offset.unwrap().abs();
let current_slot = epoch_schedule.slots_per_epoch + ancient_append_vec_offset as u64 + 1;
// Simulate killing of the ancient account by overwriting it in the current slot.
db.store_for_tests(
current_slot,
&[(&modified_account_pubkey, &modified_account)],
);
db.calculate_accounts_delta_hash(current_slot);
db.add_root_and_flush_write_cache(current_slot);
// This should remove the dead ancient account from the index.
db.clean_accounts_for_tests();
db.shrink_ancient_slots(&epoch_schedule);
let storage = db.get_storage_for_slot(starting_ancient_slot).unwrap();
let created_accounts = db.get_unique_accounts_from_storage(&storage);
// The dead account should still be in the ancient storage,
// because the storage wouldn't be shrunk with normal alive to
// capacity ratio.
assert_eq!(created_accounts.stored_accounts.len(), 3);
db.shrink_candidate_slots(&epoch_schedule);
let storage = db.get_storage_for_slot(starting_ancient_slot).unwrap();
let created_accounts = db.get_unique_accounts_from_storage(&storage);
// At this point the dead ancient account should be removed
// and storage capacity shrunk to the sum of alive bytes of
// accounts it holds. This is the data lengths of the
// accounts plus the length of their metadata.
assert_eq!(created_accounts.capacity, 1000 + 2000 + 136 * 2);
}

#[test]
fn test_select_candidates_by_total_usage_no_candidates() {
// no input candidates -- none should be selected
Expand Down
28 changes: 14 additions & 14 deletions accounts-db/src/accounts_db/stats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,18 +406,6 @@ impl ShrinkStats {
if self.last_report.should_update(1000) {
datapoint_info!(
"shrink_stats",
(
"ancient_slots_added_to_shrink",
self.ancient_slots_added_to_shrink
.swap(0, Ordering::Relaxed),
i64
),
(
"ancient_bytes_added_to_shrink",
self.ancient_bytes_added_to_shrink
.swap(0, Ordering::Relaxed),
i64
),
(
"num_slots_shrunk",
self.num_slots_shrunk.swap(0, Ordering::Relaxed) as i64,
Expand Down Expand Up @@ -519,6 +507,11 @@ impl ShrinkStats {
self.accounts_loaded.swap(0, Ordering::Relaxed) as i64,
i64
),
(
"initial_candidates_count",
self.initial_candidates_count.swap(0, Ordering::Relaxed),
i64
),
Comment on lines +510 to +514

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can all the changes to this file be reverted?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there's no functional changes, just moving the lines. But I still had to look and spend time to realize there were not actual changes to the metrics. IMO feels orthogonal to the problem that this PR is solving. Or in other words, I don't understand why this file was changed.

(
"purged_zero_lamports_count",
self.purged_zero_lamports.swap(0, Ordering::Relaxed),
Expand All @@ -535,8 +528,15 @@ impl ShrinkStats {
i64
),
(
"initial_candidates_count",
self.initial_candidates_count.swap(0, Ordering::Relaxed),
"ancient_slots_added_to_shrink",
self.ancient_slots_added_to_shrink
.swap(0, Ordering::Relaxed),
i64
),
(
"ancient_bytes_added_to_shrink",
self.ancient_bytes_added_to_shrink
.swap(0, Ordering::Relaxed),
i64
),
);
Expand Down
11 changes: 10 additions & 1 deletion accounts-db/src/ancient_append_vecs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ struct AncientSlotInfos {
total_alive_bytes_shrink: Saturating<u64>,
/// total alive bytes across all slots
total_alive_bytes: Saturating<u64>,
/// best_slots_to_shrink
best_slots_to_shrink: Vec<(Slot, u64)>,
}

impl AncientSlotInfos {
Expand Down Expand Up @@ -177,8 +179,10 @@ impl AncientSlotInfos {
* tuning.percent_of_alive_shrunk_data
/ 100,
);
self.best_slots_to_shrink = Vec::with_capacity(self.shrink_indexes.len());
for info_index in &self.shrink_indexes {
let info = &mut self.all_infos[*info_index];
self.best_slots_to_shrink.push((info.slot, info.capacity));
HaoranYi marked this conversation as resolved.
Show resolved Hide resolved
if bytes_to_shrink_due_to_ratio.0 >= threshold_bytes {
// we exceeded the amount to shrink due to alive ratio, so don't shrink this one just due to 'should_shrink'
// It MAY be shrunk based on total capacity still.
Expand Down Expand Up @@ -396,7 +400,12 @@ impl AccountsDb {
self.shrink_ancient_stats
.slots_considered
.fetch_add(sorted_slots.len() as u64, Ordering::Relaxed);
let ancient_slot_infos = self.collect_sort_filter_ancient_slots(sorted_slots, &tuning);
let mut ancient_slot_infos = self.collect_sort_filter_ancient_slots(sorted_slots, &tuning);

std::mem::swap(
&mut *self.best_ancient_slots_to_shrink.write().unwrap(),
&mut ancient_slot_infos.best_slots_to_shrink,
);
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved

if ancient_slot_infos.all_infos.is_empty() {
return; // nothing to do
Expand Down
Loading