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

stop page aligning shrunk append vecs #34016

Merged
merged 4 commits into from
Nov 13, 2023
Merged

Conversation

jeffwashington
Copy link
Contributor

Problem

There's been an effort underway to make append vecs the correct size. Among other benefits, this allows readers to not have to be told how many bytes are valid in an append vec.

I found a new place where we pad the size of an append vec to a page size.

Summary of Changes

Remove alignment to a page size when we're shrinking.

Fixes #

@@ -4114,7 +4112,7 @@ impl AccountsDb {
self.shrink_collect::<AliveAccounts<'_>>(store, &unique_accounts, &self.shrink_stats);

// This shouldn't happen if alive_bytes/approx_stored_count are accurate
if Self::should_not_shrink(shrink_collect.aligned_total_bytes, shrink_collect.capacity) {
if Self::should_not_shrink(shrink_collect.alive_total_bytes as u64, shrink_collect.capacity) {
Copy link
Contributor Author

@jeffwashington jeffwashington Nov 10, 2023

Choose a reason for hiding this comment

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

I'm not really sure what this check is doing. I think it may be saying we shouldn't shrink if the (previously aligned) alive bytes plus a page are bigger than the capacity of the current append vec. So, only shrink if we save at least a page? It adds a page size to alive bytes and compares to the capacity of the original append vec. This data point does not appear to be present on mnb, indicating alive_bytes and approx_stored_count are correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This call to should_not_shrink is the same call we made prior to trying to shrink this append vec. This is a sanity check to make sure we still think we should've shrunk this one.

@HaoranYi
Copy link
Contributor

HaoranYi commented Nov 10, 2023

Changes look good. I think there are a few more tests need to be fixed with the rename of the member var.

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #34016 (5494e8a) into master (69ab8a8) will decrease coverage by 0.1%.
Report is 13 commits behind head on master.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #34016     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         811      811             
  Lines      219412   219402     -10     
=========================================
- Hits       179792   179732     -60     
- Misses      39620    39670     +50     

@HaoranYi HaoranYi marked this pull request as ready for review November 13, 2023 17:32
@HaoranYi
Copy link
Contributor

I have fixed the build and tests for the PR.
@brooksprumo @jeffwashington Can you take a look?

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm

@HaoranYi HaoranYi self-assigned this Nov 13, 2023
@HaoranYi HaoranYi added the automerge Merge this Pull Request automatically once CI passes label Nov 13, 2023
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Nov 13, 2023
Copy link
Contributor

mergify bot commented Nov 13, 2023

automerge label removed due to a CI failure

@jeffwashington
Copy link
Contributor Author

lgtm. ty @HaoranYi

@HaoranYi HaoranYi merged commit a25eae4 into solana-labs:master Nov 13, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants