-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ancient append vec append creates big enough ancient append vecs #34154
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #34154 +/- ##
========================================
Coverage 81.9% 81.9%
========================================
Files 819 819
Lines 219741 219907 +166
========================================
+ Hits 180062 180267 +205
+ Misses 39679 39640 -39 |
20a61b3
to
639eff7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there testing coverage for shrinking an existing larger ancient append vec into the new smaller size? I see there's a test that we can create the larger sized ancient append vecs.
I think it is covered by the second test. When we try to create a new ancient append vec from a previous one with a larger size, it will keep the larger size of the old ancient vec so that it can fits all the accounts from the original ancient appendvec. |
More tests are always better! |
When we call `remaining_bytes` to calculate, for the last element, u64_align!(len) could be greater than capacity. However, because saturate_sub is used, we still get the correct answer of 0. Therefore, no need to align the storage to u64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
let size: usize = aligned_stored_size(account_data_size.unwrap_or(123) as usize); | ||
let mut data = AccountStorageEntry::new(&paths[0], slot, id, size as u64); | ||
let av = AccountsFile::AppendVec(AppendVec::new(&tf.path, true, (1024 * 1024).max(size))); | ||
let file_size = account_data_size.unwrap_or(123) * 100 / fill_percentage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HaoranYi why is this * 100 / fill_percentage
. I'm struggling to get the math here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's some fancy markup. ty
Problem
When appending ancient append vecs, an implicit assumption was being made that the ideal ancient append vec size could hold the contents of any existing append vec. This is not a valid assumption. We are adjusting the ideal size to be smaller than it has been. This change illustrated how a too large ancient append vec could cause an assertion if we tried to re-append it.
Summary of Changes
When appending, allocate ancient append vecs large enough to contain the specific requirements for the current operation.
Fixes #