Increase the maximum of ancient slots allowed to be not combined #3205
Conversation
|
I'm working on adding an option to set this parameter on the validator's command line. |
|
I suggest coupling this change with: This keeps the overall 'ancient' range at 100k max, and the overall storage range at 432k, so we are at status quo relative today regarding # slots and # file handles. |
I think 100k means move it into the 432k. I think -10k moves it past the end (so max of 432k - - 10k = 442k. I'm proposing 432k - 100k + 100k = 432k total. |
|
Ah. I see. So we will update this in the next PR? agave/accounts-db/src/accounts_db.rs Line 598 in 369d2cb |
I wanted that done IN this pr: I guess I could've been more clear. |
in this one. |
accounts-db/src/accounts_db.rs
Outdated
| @@ -595,7 +595,7 @@ pub struct AccountsAddRootTiming { | |||
|
|
|||
| /// 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); | |||
| const ANCIENT_APPEND_VEC_DEFAULT_OFFSET: Option<i64> = Some(-100_000); | |||
There was a problem hiding this comment.
shouldn't it be an offset to smaller slot numbers? if the offset is positive, the logic of several tests has to be adjusted at least, maybe some logic in the algorithms as well. let me figure this out.
|
we definitely need to start a skipping rewrites version of this and it needs to run more than an epoch. Skipping rewrites is still opt-in and the current master behavior is unacceptable, so even if this isn't completely right, the risk is low at the moment. |
|
maybe I should merge #3206, rebase this on new master, and then start a skipping rewrites validator? so that all tweaks are included... |
| /// 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); |
There was a problem hiding this comment.
Should MAX_ANCIENT_SLOTS_DEFAULT be used here?
| 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?
There was a problem hiding this comment.
i think they may have different values, in theory...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can we include the discussion here in the code comments, i.e. document the relationship between these two constants?
There was a problem hiding this comment.
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).
| /// 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); |
There was a problem hiding this comment.
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.
accounts-db/src/accounts_db.rs
Outdated
| @@ -15877,16 +15888,19 @@ pub mod tests { | |||
| assert!(db | |||
| .get_sorted_potential_ancient_slots(oldest_non_ancient_slot) | |||
| .is_empty()); | |||
| let root0 = 0; | |||
| let root0 = MAX_ANCIENT_SLOTS_DEFAULT as u64 + ancient_append_vec_offset as u64 + 1; | |||
There was a problem hiding this comment.
nit: I find it confusing to have a variable named root0 but not have a value of 0. Often we have slotX or rootX where the X is the slot number. Here we're breaking that pattern, and thus adds unexpectedness (which then I try and figure out "why"). I think we could rename the variable and bypass this issue entirely.
Problem
Large files containing ancient slots are expensive to update when the contained account data is being overwritten in more recent slots.
Summary of Changes
We increase the value of the tuning parameter
max_ancient_slotsto allow more ancient slots to remain in their individual storages instead of being combined into larger files. The smaller files are faster to update at the expense of the validator using more file handles.