Refine the condition for 'should shrink' decision #3252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brooksprumo
reviewed
Oct 23, 2024
Ah ok, I looked at the code and now see: by default the shrink threshold is with TotalSpace, not IndividualStore. |
brooksprumo
reviewed
Oct 23, 2024
HaoranYi
approved these changes
Oct 23, 2024
HaoranYi
reviewed
Oct 23, 2024
| @@ -7518,7 +7518,7 @@ impl AccountsDb { | |||
| true | |||
| } | |||
|
|
|||
| fn is_candidate_for_shrink(&self, store: &AccountStorageEntry) -> bool { | |||
| pub(crate) fn is_candidate_for_shrink(&self, store: &AccountStorageEntry) -> bool { | |||
There was a problem hiding this comment.
Can we document this fn?
Suggested change
| pub(crate) fn is_candidate_for_shrink(&self, store: &AccountStorageEntry) -> bool { | |
| /// Determines whether a given AccountStorageEntry instance is a candidate for shrinking. | |
| pub(crate) fn is_candidate_for_shrink(&self, store: &AccountStorageEntry) -> bool { |
There was a problem hiding this comment.
I'll make a follow up pr with the documentation. Good point, though. Thanks!
brooksprumo
approved these changes
Oct 23, 2024
ray-kast
pushed a commit
to abklabs/agave
that referenced
this pull request
Nov 27, 2024
* Refine the condition for 'should shrink' decision * Fix tests * Fix clippy * Brooks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
When the ancient packing algorithm was designed, the intention was for 'shrinking' to occur as part of packing. This was observed to cause cold and warm accounts to be continually mixed, resulting in lots of disk i/o churn. In the newer strategy, we don't combine previously packed ancient storages with newly ancient storages. We do, however, rely on shrinking to shrink ancient storages. An important concept is that older ancient storages are very cold. Thus, the accounts in them are not expected to change frequently. if we require a threshold, like < 90% accounts are alive, some of these storages will never fail the threshold and be eligible for shrinking. When shrinking is idle, it finds the ancient storage with the highest # dead bytes and shrinks it.
Summary of Changes
Ancient packing adds ALL storages with dead bytes to the list that shrink uses to determine which ancient storages to shrink when shrink is otherwise idle.