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

Avoid combining older slots with newer ones in ancient shrinking #3189

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

dmakarov
Copy link

@dmakarov dmakarov commented Oct 16, 2024

Problem

shrink_ancient_slots bundles ancient storages together without considering the relative age of slots for which they contain account data. The combined storage can contain account data for very old slots along with the data for more recent slots, thus making it more likely that the combined storage will have to be updated soon. More uniform storages w.r.t. to the age of slots they contain would less likely have to be updated when account data is overwritten in newer slots.

Summary of Changes

This change tweaks a tuning parameter that determines which ancient slots are eligible for shrinking, and, in the process, for combining into a larger storage. Setting the percent_of_alive_shrunk_data parameter to 0, we effectively disable selecting old ancient slots for shrinking based only on their contribution of alive bytes to the combined storage alive bytes. These slots can still be picked up for shrinking when shrink_candidate_slots is called, but their storages won't be combined with the storages containing newer slots account data unless these older storages become small.

@dmakarov dmakarov force-pushed the shrink branch 6 times, most recently from e873ea5 to 59de09f Compare October 16, 2024 21:16
@dmakarov dmakarov changed the title Defer ancient slots shrinking to shrink_candidate_slots Avoid combining older slots with newer ones in ancient shrinking Oct 16, 2024
@dmakarov
Copy link
Author

If the tuning parameter percent_of_alive_shrunk_data is going to be 0, maybe it's better to remove it completely and change the logic of the algorithm accordingly?

@jeffwashington
Copy link

If the tuning parameter percent_of_alive_shrunk_data is going to be 0, maybe it's better to remove it completely and change the logic of the algorithm accordingly?

Yes. But we are still finding a long term equilibrium. I THINK we will end up ripping out this shrinking selection behavior. But, in the meantime, it could be very convenient to have this already present.

@dmakarov dmakarov marked this pull request as ready for review October 16, 2024 22:40
Copy link

@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.

How does this perform on a mnb validator that is skipping rewrites?

jeffwashington
jeffwashington previously approved these changes Oct 17, 2024
Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm. normal shrink will take care of these.

@jeffwashington
Copy link

How does this perform on a mnb validator that is skipping rewrites?

i've been testing this for many weeks. It does what we want. We accumulate old, cold storages and newer things are combined together. We may be shrinking too much (too many bytes of i/o and index churn for too little savings), but that is tuning to the shrinking algorithm we can do. This change stops ancient packing from also shrinking older storages. When shrink is idle, it shrinks the most productive ancient storage with dead bytes.

@dmakarov
Copy link
Author

How does this perform on a mnb validator that is skipping rewrites?

I started a validator with --accounts-db-test-skip-rewrites command line option on my dev machine. Let's see in a couple of days?

brooksprumo
brooksprumo previously approved these changes Oct 17, 2024
Copy link

@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.

:shipit:

Jwash has been testing, so I feel good with merging.

jeffwashington
jeffwashington previously approved these changes Oct 17, 2024
Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

brooksprumo
brooksprumo previously approved these changes Oct 17, 2024
Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

Seems like it is good to remove the byte-saving heuristic from ancient pack.
Naturally, this heuristic should be owned by shrinking.

We have too many heuristics considered in ancient packing, - by higher slots,
by smaller size and by byte-savings.

Too many heuristics make reasoning hard. Those heuristics may also be conflict
with each other as described in the pr description.

Now leaving byte-saving bytes out of the picture of ancient packing and let
shrinking deal with byte-savings seems to be a good decoupling and make the
overall reasoning and tuning simpler.

lgtm

@@ -3786,7 +3788,7 @@ pub mod tests {
// only allow 10k slots old enough to be ancient
max_ancient_slots: 10_000,
// re-combine/shrink 55% of the data savings this pass

Choose a reason for hiding this comment

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

nit: one more outdated comment.

Copy link
Author

Choose a reason for hiding this comment

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

removed. thank you.

@dmakarov dmakarov dismissed stale reviews from brooksprumo and jeffwashington via 3bb5af2 October 17, 2024 19:22
@dmakarov dmakarov requested a review from HaoranYi October 17, 2024 19:22
@jeffwashington jeffwashington self-requested a review October 17, 2024 20:21
Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@dmakarov dmakarov merged commit 369d2cb into anza-xyz:master Oct 17, 2024
40 checks passed
@dmakarov dmakarov deleted the shrink branch October 17, 2024 21:55
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
…a-xyz#3189)

* Avoid combining older slots with newer ones in ancient shrinking

* Comment

* Comment
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.

4 participants