Replace purges_old_accounts by a boolean field in CleaningInfo #2566
Conversation
| let reclaim_vecs = candidates | ||
| .par_iter() | ||
| .filter_map(|candidates_bin| { |
accounts-db/src/accounts_db.rs
Outdated
| max_clean_root_inclusive: Option<Slot>, | ||
| ancient_account_cleans: &AtomicU64, | ||
| epoch_schedule: &EpochSchedule, | ||
| ) -> (ReclaimResult, PubkeysRemovedFromAccountsIndex) { | ||
| let pubkeys_removed_from_accounts_index = HashSet::default(); | ||
| if purges.is_empty() { | ||
| // return early if there are no old accounts to purge | ||
| if !candidates |
There was a problem hiding this comment.
this could be very expensive and I think will scan all bins every time. I'd prefer to see a bool set at the caller that knows if there were any should_purge accounts. You could set a local atomic in the index scan in the calling fn if any account is set to should_purge. That could be passed to this fn or this fn could just assume that there is at least one should_purge entry if it is called with candidates. This first check is really just a cya to make sure that we know the behavior if there are no should_purge accounts. So, we could probably get rid of it in any case if we are careful of what the resulting behavior would be. I acknowledge that this code is a correct drop in replacement for purges before.
There was a problem hiding this comment.
I moved the check to the caller and used an atomic bool to record whether any purgeable candidate exists.
|
Yes! I love this. ty! |
accounts-db/src/accounts_db.rs
Outdated
| let mut not_found_on_fork = 0; | ||
| let mut missing = 0; | ||
| let mut useful = 0; | ||
| let mut exist_purgeable_accounts_local = false; |
There was a problem hiding this comment.
sorry to go round and round. Let's simplify this. In practice, there are ALWAYS a non zero # of accounts to clean.
so, no need to check. Just always call the fn. And make sure it handles no accounts, but that is easy to do on inspection.
btw, this graph is min, so there are never 0 entries.
There was a problem hiding this comment.
I removed the check. From what I can tell the function should return empty ReclaimResult and an empty HashSet for pubkeys_removed_from_accounts_index if there were no purgeable accounts.
There was a problem hiding this comment.
i read through it as well. it appears as if it will do the right thing.
We added 4 bytes (a |
yep, I thought about the 4 bytes. i expected a different curve. doesn't mean this is right or wrong - just surprising. |
I think this is a good idea anyway. |
do this separately. this pr is ready imo |
unless we can think of a clever way to store the bit in what we're already storing. |
Let's merge this for now, and I'll think how it might be improved. |
…xyz#2566) * Replace purges_old_accounts by a boolean field in CleaningInfo * comments * Move check for purgeable accounts existence to caller * Corrections to Atomic * correction. * Remove checks for no-existent purge old accounts
Problem
Cleaning empty accounts from accounts index uses a lot of memory by copying large amounts of data.
Summary of Changes
Further reduce memory use in
clean_accountsby keeping information about cleaning candidates in a single data structure instead of copying it to multiple containers. This PR removespurges_old_accountsvector and replaces it with a boolean fieldshould_purgeinCleaningInfostructure.