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

Increase the maximum of ancient slots allowed to be not combined #3205

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 31 additions & 17 deletions accounts-db/src/accounts_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ const SHRINK_COLLECT_CHUNK_SIZE: usize = 50;
/// candidates for shrinking.
const SHRINK_INSERT_ANCIENT_THRESHOLD: usize = 10;

/// Default value for the number of ancient storages the ancient slot
/// combining should converge to.
pub const MAX_ANCIENT_SLOTS_DEFAULT: usize = 100_000;

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
pub enum CreateAncientStorage {
/// ancient storages are created by appending
Expand Down Expand Up @@ -593,9 +597,17 @@ pub struct AccountsAddRootTiming {
pub store_us: u64,
}

/// if negative, this many accounts older than # slots in epoch are still treated as modern (ie. non-ancient).
/// Slots older than # slots in epoch - this # are then treated as ancient and subject to packing.
const ANCIENT_APPEND_VEC_DEFAULT_OFFSET: Option<i64> = Some(-10_000);
/// Slots older the "number of slots in an epoch minus this number"
/// than max root are treated as ancient and subject to packing.
/// | older |<- slots in an epoch ->| max root
/// | older |<- offset ->| |
/// | ancient | modern |
///
/// If this is negative, this many slots older than the number of
/// slots in epoch are still treated as modern (ie. non-ancient).
/// | older |<- abs(offset) ->|<- slots in an epoch ->| max root
/// | ancient | modern |
const ANCIENT_APPEND_VEC_DEFAULT_OFFSET: Option<i64> = Some(100_000);

Choose a reason for hiding this comment

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

Should MAX_ANCIENT_SLOTS_DEFAULT be used here?

Suggested change
const ANCIENT_APPEND_VEC_DEFAULT_OFFSET: Option<i64> = Some(100_000);
const ANCIENT_APPEND_VEC_DEFAULT_OFFSET: Option<i64> = Some(MAX_ANCIENT_SLOTS_DEFAULT);

Do we need two constants?

Copy link
Author

Choose a reason for hiding this comment

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

i think they may have different values, in theory...

Choose a reason for hiding this comment

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

Ok, I think I'm fine with two constants. I still think that by default we probably want the offset to be the same as the max number of ancient slots.

Copy link
Author

Choose a reason for hiding this comment

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

i believe they’re not related, that is changing one value shouldn’t affect the other. otherwise why should there be two constants, instead of one? Perhaps, follow up changes should reconsider the tuning parameters altogether.

Choose a reason for hiding this comment

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

Yeah, that's fair. I guess I was under the impression that by setting the offset to a positive value, and setting the max number of ancient slots be the same value, that we were implicitly trying to communicate that we wanted a maximum number of storages to be about the number of slots per epoch.

Choose a reason for hiding this comment

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

Can we include the discussion here in the code comments, i.e. document the relationship between these two constants?

Copy link
Author

Choose a reason for hiding this comment

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

by the way max_ancient_slots seems to be a misnomer, because the value designates the number of storage files, not slots (there are in general accounts data from more slots packed into this number of storage files, if I understand correctly).


#[derive(Debug, Default, Clone)]
pub struct AccountsDbConfig {
Expand Down Expand Up @@ -15146,10 +15158,9 @@ pub mod tests {
let slots_per_epoch = config.epoch_schedule.slots_per_epoch;
assert_ne!(slot, 0);
let offset = 10;
// no ancient append vecs, so always 0
assert_eq!(
db.get_oldest_non_ancient_slot_for_hash_calc_scan(slots_per_epoch + offset, &config),
expected(0)
expected(db.ancient_append_vec_offset.unwrap() as u64 + offset + 1)
);
// ancient append vecs enabled (but at 0 offset), so can be non-zero
db.ancient_append_vec_offset = Some(0);
Expand Down Expand Up @@ -15877,52 +15888,55 @@ pub mod tests {
assert!(db
.get_sorted_potential_ancient_slots(oldest_non_ancient_slot)
.is_empty());
let root0 = 0;
db.add_root(root0);
let root1 = 1;
let root1 = MAX_ANCIENT_SLOTS_DEFAULT as u64 + ancient_append_vec_offset as u64 + 1;
db.add_root(root1);
let root2 = root1 + 1;
db.add_root(root2);
let oldest_non_ancient_slot = db.get_oldest_non_ancient_slot(&epoch_schedule);
assert!(db
.get_sorted_potential_ancient_slots(oldest_non_ancient_slot)
.is_empty());
let completed_slot = epoch_schedule.slots_per_epoch;
db.accounts_index.add_root(completed_slot);
db.accounts_index.add_root(AccountsDb::apply_offset_to_slot(
completed_slot,
ancient_append_vec_offset,
));
let oldest_non_ancient_slot = db.get_oldest_non_ancient_slot(&epoch_schedule);
// get_sorted_potential_ancient_slots uses 'less than' as opposed to 'less or equal'
// so, we need to get more than an epoch away to get the first valid root
assert!(db
.get_sorted_potential_ancient_slots(oldest_non_ancient_slot)
.is_empty());
let completed_slot = epoch_schedule.slots_per_epoch + root0;
let completed_slot = epoch_schedule.slots_per_epoch + root1;
db.accounts_index.add_root(AccountsDb::apply_offset_to_slot(
completed_slot,
-ancient_append_vec_offset,
ancient_append_vec_offset,
));
let oldest_non_ancient_slot = db.get_oldest_non_ancient_slot(&epoch_schedule);
assert_eq!(
db.get_sorted_potential_ancient_slots(oldest_non_ancient_slot),
vec![root0]
vec![root1, root2]
);
let completed_slot = epoch_schedule.slots_per_epoch + root1;
let completed_slot = epoch_schedule.slots_per_epoch + root2;
db.accounts_index.add_root(AccountsDb::apply_offset_to_slot(
completed_slot,
-ancient_append_vec_offset,
ancient_append_vec_offset,
));
let oldest_non_ancient_slot = db.get_oldest_non_ancient_slot(&epoch_schedule);
assert_eq!(
db.get_sorted_potential_ancient_slots(oldest_non_ancient_slot),
vec![root0, root1]
vec![root1, root2]
);
db.accounts_index
.roots_tracker
.write()
.unwrap()
.alive_roots
.remove(&root0);
.remove(&root1);
let oldest_non_ancient_slot = db.get_oldest_non_ancient_slot(&epoch_schedule);
assert_eq!(
db.get_sorted_potential_ancient_slots(oldest_non_ancient_slot),
vec![root1]
vec![root2]
);
});

Expand Down
6 changes: 4 additions & 2 deletions accounts-db/src/ancient_append_vecs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use {
stats::{ShrinkAncientStats, ShrinkStatsSub},
AccountFromStorage, AccountStorageEntry, AccountsDb, AliveAccounts,
GetUniqueAccountsResult, ShrinkCollect, ShrinkCollectAliveSeparatedByRefs,
MAX_ANCIENT_SLOTS_DEFAULT,
},
accounts_file::AccountsFile,
active_stats::ActiveStatItem,
Expand Down Expand Up @@ -342,8 +343,9 @@ impl AccountsDb {
can_randomly_shrink: bool,
) {
let tuning = PackedAncientStorageTuning {
// only allow 10k slots old enough to be ancient
max_ancient_slots: 10_000,
// Slots old enough to be ancient. Setting this parameter
// to 100k makes ancient storages to be approx 5M.
max_ancient_slots: MAX_ANCIENT_SLOTS_DEFAULT,
// Don't re-pack anything just to shrink.
// shrink_candidate_slots will handle these old storages.
percent_of_alive_shrunk_data: 0,
Expand Down
Loading