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.

Sign up for GitHub

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

tune ancient append vec size to 128M #34067

Merged
merged 4 commits into from
Dec 11, 2023
Merged

tune ancient append vec size to 128M #34067

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37be68d
tune ancient append vec size to 130M
Nov 14, 2023
2f4af3f
fix a test and get rid of the assert since it is covered in the test
Nov 15, 2023
9d35709
use 128M
Nov 15, 2023
eb5dfc8
assert max append vec size for ancient append vec
Nov 16, 2023
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
16 changes: 10 additions & 6 deletions accounts-db/src/ancient_append_vecs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,13 +918,17 @@ impl<'a> AccountsToStore<'a> {
}

/// capacity of an ancient append vec
pub fn get_ancient_append_vec_capacity() -> u64 {
#[allow(clippy::assertions_on_constants)]
pub const fn get_ancient_append_vec_capacity() -> u64 {
use crate::append_vec::MAXIMUM_APPEND_VEC_FILE_SIZE;
// smaller than max by a bit just in case
// some functions add slop on allocation
// The bigger an append vec is, the more unwieldy it becomes to shrink, create, write.
// 1/10 of max is a reasonable size in practice.
MAXIMUM_APPEND_VEC_FILE_SIZE / 10 - 2048
const PAGE_SIZE: u64 = 4 * 1024;

// There is a trade-off for selecting the ancient append vec size. Too small size will result in
// too many ancient append vec memory mapped files. Too big size will make it difficult to clean and shrink.
// Hence, we choose approximately 130MB (i.e. 134,217,728 bytes) for the ancient append vec size.
const RESULT: u64 = PAGE_SIZE * 32768;
Copy link
Contributor

@brooksprumo brooksprumo Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

One reason for choosing 128 MiB is that it's a power of two and then we can steal bits off the top of offsets into these files. I think it could be useful to include that in the "why" comments. (We can say 128 MiB too, instead of approximately 130 MB.)

Other options for specifying the size are:

  • 128 * 1024 * 1024 (makes it obvious the size is 128 MiB)
  • 1 << 27 (makes it obvious we have 5 bits to spare on a u32)

For the asserts, we could also add why it's necessary that ancient append vecs are smaller than non-ancient. (Just as a comment, not within the assert!(). Same for why we want a multiple of 4 KiB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds reasonable to me. @jeffwashington wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with however you want to specify the size.
I'd like to see the assert remain so we connect these two 'constants'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. updated.

const _: () = assert!(RESULT < MAXIMUM_APPEND_VEC_FILE_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

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

@HaoranYi I like this assert. The exact value is covered in the test. But, it is helpful to find the places where MAXIMUM_APPEND_VEC_FILE_SIZE is used in case of future changes by future 'us'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. added.

RESULT
}

/// is this a max-size append vec designed to be used as an ancient append vec?
Expand Down