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

clean scan optimization: scan disk index only for zero lamport #2879

Merged
merged 8 commits into from
Oct 3, 2024
Merged

clean scan optimization: scan disk index only for zero lamport #2879

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5cda868
clean scan optimization
Sep 9, 2024
26f77fb
fix rebase conflicts
Sep 24, 2024
84263bf
Update accounts-db/src/accounts_db.rs
HaoranYi Sep 25, 2024
07ae15d
Update accounts-db/src/accounts_db.rs
HaoranYi Sep 25, 2024
d245937
Update accounts-db/src/accounts_db.rs
HaoranYi Sep 25, 2024
a3333ab
Update accounts-db/src/accounts_db.rs
HaoranYi Sep 25, 2024
e45cf24
review update
Sep 25, 2024
9f676c4
revert ZeroLamport trait for IndexInfoInner
Sep 25, 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
57 changes: 45 additions & 12 deletions accounts-db/src/accounts_db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1352,6 +1352,10 @@ impl StoreAccountsTiming {
struct CleaningInfo {
slot_list: SlotList<AccountInfo>,
ref_count: u64,
/// Indicates if this account might have a zero lamport index entry.
/// If false, the account *shall* not have zero lamport index entries.
/// If true, the account *might* have zero lamport index entries.
might_contain_zero_lamport_entry: bool,
}

/// This is the return type of AccountsDb::construct_candidate_clean_keys.
Expand Down Expand Up @@ -2849,6 +2853,7 @@ impl AccountsDb {
CleaningInfo {
slot_list,
ref_count,
..
},
) in bin.iter()
{
Expand Down Expand Up @@ -3105,7 +3110,18 @@ impl AccountsDb {
candidates_bin = candidates[curr_bin].write().unwrap();
prev_bin = curr_bin;
}
candidates_bin.insert(removed_pubkey, CleaningInfo::default());
// Conservatively mark the candidate might have a zero lamport entry for
// correctness so that scan WILL try to look in disk if it is
// not in-mem. These keys are from 1) recently processed
// slots, 2) zero lamports found in shrink. Therefore, they are very likely
// to be in-memory, and seldomly do we need to look them up in disk.
candidates_bin.insert(
removed_pubkey,
CleaningInfo {
might_contain_zero_lamport_entry: true,
..Default::default()
},
);
}
}
}
Expand All @@ -3119,12 +3135,6 @@ impl AccountsDb {
.sum::<usize>() as u64
}

fn insert_pubkey(&self, candidates: &[RwLock<HashMap<Pubkey, CleaningInfo>>], pubkey: Pubkey) {
let index = self.accounts_index.bin_calculator.bin_from_pubkey(&pubkey);
let mut candidates_bin = candidates[index].write().unwrap();
candidates_bin.insert(pubkey, CleaningInfo::default());
}

/// Construct a list of candidates for cleaning from:
/// - dirty_stores -- set of stores which had accounts
/// removed or recently rooted;
Expand Down Expand Up @@ -3162,6 +3172,16 @@ impl AccountsDb {
std::iter::repeat_with(|| RwLock::new(HashMap::<Pubkey, CleaningInfo>::new()))
.take(num_bins)
.collect();

let insert_candidate = |pubkey, is_zero_lamport| {
let index = self.accounts_index.bin_calculator.bin_from_pubkey(&pubkey);
let mut candidates_bin = candidates[index].write().unwrap();
candidates_bin
.entry(pubkey)
.or_default()
.might_contain_zero_lamport_entry |= is_zero_lamport;
};

let dirty_ancient_stores = AtomicUsize::default();
let mut dirty_store_routine = || {
let chunk_size = 1.max(dirty_stores_len.saturating_div(rayon::current_num_threads()));
Expand All @@ -3174,9 +3194,12 @@ impl AccountsDb {
dirty_ancient_stores.fetch_add(1, Ordering::Relaxed);
}
oldest_dirty_slot = oldest_dirty_slot.min(*slot);
store
.accounts
.scan_pubkeys(|pubkey| self.insert_pubkey(&candidates, *pubkey));

store.accounts.scan_index(|index| {
Copy link

Choose a reason for hiding this comment

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

Ah, I missed this the first time. We're using scan_index() now, instead of scan_pubkeys(). I think this is fine. With AppendVecs this is basically no additional cost. With Tiered Storage, it shouldn't be much additional work to get the is-zero-lamports information (esp once tiered storage has the lamports optimizations).

Copy link
Author

Choose a reason for hiding this comment

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

yeah. we need to read the accounts meta. but it shouldn't be expensive.

let pubkey = index.index_info.pubkey;
let is_zero_lamport = index.index_info.lamports == 0;
insert_candidate(pubkey, is_zero_lamport);
Comment on lines +3200 to +3201
Copy link

Choose a reason for hiding this comment

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

As we're scanning here, what happens in the case where this index_info is not zero lamport, but there is another index entry for this pubkey that is zero lamport? If we end up with setting might_contain_zero_lamport_entry to false, will that prevent the later scan from looking on disk and finding the other zero lamport entry?

Copy link
Author

@HaoranYi HaoranYi Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No. if it was true, then it will still be true, I think. We are oring is_zero_lamport.

            candidates_bin
                .entry(pubkey)
                .or_default()
                .might_contain_zero_lamport_entry |= is_zero_lamport;

Copy link

Choose a reason for hiding this comment

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

Yes, I understand if a different entry for the same pubkey sets might_contain_zero_lamport_entry to true, then other falses will not reset the field to false.

My question is, when looking at the dirty stores, what if there's a pubkey A in dirty storage slot 100, with non-zero lamports. Also assume pubkey A is in non dirty storage slot 7 with zero lamports. It looks to me that we'd only look at storage 100 and not storage 7, so the clean candidate would say "nope, no zero lamport entries here" and then the scan filter would not look on disk for the other index entries.

I think after typing that out, I can see why it won't happen. If we have a dirty storage at slot 100, then the index entry for pubkey A must be in the in-memory index, as the slot-list will be 2 in this example. So only looking in-mem is safe, and we don't need to look on disk.

Does that sound right?

Copy link
Author

Choose a reason for hiding this comment

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

Ah. I see what you mean.
Yes, you are correct. If there are other instances of the account outside of the stores that we scan, then disk index is irrelevant because it will always be in memory.

});
});
oldest_dirty_slot
})
Expand Down Expand Up @@ -3226,7 +3249,7 @@ impl AccountsDb {
let is_candidate_for_clean =
max_slot_inclusive >= *slot && latest_full_snapshot_slot >= *slot;
if is_candidate_for_clean {
self.insert_pubkey(&candidates, *pubkey);
insert_candidate(*pubkey, true);
Copy link

Choose a reason for hiding this comment

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

why is this 'true'?

Copy link

Choose a reason for hiding this comment

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

ah, because we are iterating zero_lamport_accounts_to_purge_after_full_snapshot

Copy link
Author

Choose a reason for hiding this comment

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

yes. exactly!

}
!is_candidate_for_clean
});
Expand Down Expand Up @@ -3442,7 +3465,11 @@ impl AccountsDb {
},
None,
false,
ScanFilter::All,
if candidate_info.might_contain_zero_lamport_entry {
ScanFilter::All
} else {
self.scan_filter_for_shrinking
Copy link

Choose a reason for hiding this comment

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

Can you remind me where self.scan_filter_for_shrinking is set (and what the value is)?

Copy link
Author

@HaoranYi HaoranYi Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/anza-xyz/agave/blob/master/validator/src/main.rs#L1274-L1285

It is set by the CLI.
default is "all", so no impact if not passed by CLI.

Maybe we should rename the CLI in a future PR, as it is not just for shrinking but also for cleaning.

Copy link

@jeffwashington jeffwashington Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

note that until we change the default of this cli arg, we'll have no impact on clean (or shrink) due to this filtering scan thing. default is ScanFilter::All

So, once this goes in, we need long term testing of setting scan filter to abnormal only.

},
);
if should_purge {
let reclaims_new = self.collect_reclaims(
Expand Down Expand Up @@ -3493,6 +3520,7 @@ impl AccountsDb {
CleaningInfo {
slot_list,
ref_count,
..
},
) in candidates_bin.write().unwrap().iter_mut()
{
Expand Down Expand Up @@ -3572,6 +3600,7 @@ impl AccountsDb {
let CleaningInfo {
slot_list,
ref_count: _,
..
} = cleaning_info;
(!slot_list.is_empty()).then_some((
*pubkey,
Expand Down Expand Up @@ -3875,6 +3904,7 @@ impl AccountsDb {
let CleaningInfo {
slot_list,
ref_count: _,
..
} = cleaning_info;
debug_assert!(!slot_list.is_empty(), "candidate slot_list can't be empty");
// Only keep candidates where the entire history of the account in the root set
Expand Down Expand Up @@ -12959,6 +12989,7 @@ pub mod tests {
CleaningInfo {
slot_list: rooted_entries,
ref_count,
..Default::default()
},
);
}
Expand All @@ -12969,6 +13000,7 @@ pub mod tests {
CleaningInfo {
slot_list: list,
ref_count,
..
},
) in candidates_bin.iter()
{
Expand Down Expand Up @@ -15221,6 +15253,7 @@ pub mod tests {
CleaningInfo {
slot_list: vec![(slot, account_info)],
ref_count: 1,
..Default::default()
},
);
let accounts_db = AccountsDb::new_single_for_tests();
Expand Down
Loading