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.

Sign up for GitHub

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

Replace purges_old_accounts by a boolean field in CleaningInfo #2566

Merged
merged 6 commits into from
Aug 14, 2024
Merged

Replace purges_old_accounts by a boolean field in CleaningInfo #2566

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
872f868
Replace purges_old_accounts by a boolean field in CleaningInfo
dmakarov Aug 12, 2024
3795b17
comments
dmakarov Aug 13, 2024
89ba157
Move check for purgeable accounts existence to caller
dmakarov Aug 13, 2024
2b5bce5
Corrections to Atomic
dmakarov Aug 13, 2024
0a3287f
correction.
dmakarov Aug 13, 2024
1717695
Remove checks for no-existent purge old accounts
dmakarov Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions accounts-db/src/accounts_db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3289,6 +3289,7 @@ impl AccountsDb {
let mut not_found_on_fork = 0;
let mut missing = 0;
let mut useful = 0;
let mut exist_purgeable_accounts_local = false;
Copy link

@jeffwashington jeffwashington Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sorry to go round and round. Let's simplify this. In practice, there are ALWAYS a non zero # of accounts to clean.
image

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.

Copy link
Author

@dmakarov dmakarov Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

i read through it as well. it appears as if it will do the right thing.

let mut candidates_bin = candidates_bin.write().unwrap();
// Iterate over each HashMap entry to
// avoid capturing the HashMap in the
Expand Down Expand Up @@ -3339,8 +3340,7 @@ impl AccountsDb {
if slot_list.len() > 1 {
// no need to purge old accounts if there is only 1 slot in the slot list
candidate_info.should_purge = true;
exist_purgeable_accounts
.store(true, Ordering::Relaxed);
exist_purgeable_accounts_local = true;
useless = false;
} else {
self.clean_accounts_stats
Expand All @@ -3358,7 +3358,7 @@ impl AccountsDb {
// touched in must be unrooted.
not_found_on_fork += 1;
candidate_info.should_purge = true;
exist_purgeable_accounts.store(true, Ordering::Relaxed);
exist_purgeable_accounts_local = true;
useless = false;
}
}
Expand All @@ -3378,7 +3378,8 @@ impl AccountsDb {
not_found_on_fork_accum.fetch_add(not_found_on_fork, Ordering::Relaxed);
missing_accum.fetch_add(missing, Ordering::Relaxed);
useful_accum.fetch_add(useful, Ordering::Relaxed);
})
exist_purgeable_accounts.store(exist_purgeable_accounts_local, Ordering::Release);
});
dmakarov marked this conversation as resolved.
Show resolved Hide resolved
};
if is_startup {
do_clean_scan();
Expand All @@ -3390,7 +3391,7 @@ impl AccountsDb {

let mut clean_old_rooted = Measure::start("clean_old_roots");
let ((purged_account_slots, removed_accounts), mut pubkeys_removed_from_accounts_index) =
if exist_purgeable_accounts.load(Ordering::Relaxed) {
if exist_purgeable_accounts.load(Ordering::Acquire) {
self.clean_accounts_older_than_root(
&candidates,
max_clean_root_inclusive,
Expand Down
Loading