tune ancient append vec size to 128M #34067
Conversation
| 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sounds reasonable to me. @jeffwashington wdyt?
There was a problem hiding this comment.
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'
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #34067 +/- ##
=========================================
- Coverage 81.9% 81.9% -0.1%
=========================================
Files 811 811
Lines 219628 219627 -1
=========================================
- Hits 179974 179941 -33
- Misses 39654 39686 +32 |
HaoranYi
changed the title
| // 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; | ||
| const _: () = assert!(RESULT < MAXIMUM_APPEND_VEC_FILE_SIZE); |
There was a problem hiding this comment.
@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'.
|
This PR will require the following PR to create big ancient append vecs. |
github-actions
bot
added
the
stale
|
Now that #34154 is merged. |
| // 1/10 of max is a reasonable size in practice. | ||
| MAXIMUM_APPEND_VEC_FILE_SIZE / 10 - 2048 | ||
| const _: () = assert!( | ||
| RESULT < MAXIMUM_APPEND_VEC_FILE_SIZE, |
There was a problem hiding this comment.
I think we should use <= here.
| RESULT < MAXIMUM_APPEND_VEC_FILE_SIZE, | |
| RESULT <= MAXIMUM_APPEND_VEC_FILE_SIZE, |
(And the assert message can be tweaked slightly to say "not greater than" or equivalent.)
There was a problem hiding this comment.
Eh... maybe the < is better. Retracting my concern here.
Problem
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.
Here, we choose 128M bytes for the ancient append vec size.
Summary of Changes
set ancient append vec size to 128M
Fixes #