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 2 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
67 changes: 54 additions & 13 deletions accounts-db/src/accounts_db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ use {
std::{
borrow::Cow,
boxed::Box,
collections::{BTreeSet, HashMap, HashSet},
collections::{hash_map::Entry, BTreeSet, HashMap, HashSet},
fs,
hash::{Hash as StdHash, Hasher as StdHasher},
io::Result as IoResult,
Expand Down Expand Up @@ -1352,6 +1352,8 @@ impl StoreAccountsTiming {
struct CleaningInfo {
slot_list: SlotList<AccountInfo>,
ref_count: u64,
/// True for pubkeys which can contain zero accounts
can_contain_zero: bool,
HaoranYi marked this conversation as resolved.
Show resolved Hide resolved
}

/// This is the return type of AccountsDb::construct_candidate_clean_keys.
Expand Down Expand Up @@ -2849,6 +2851,7 @@ impl AccountsDb {
CleaningInfo {
slot_list,
ref_count,
..
},
) in bin.iter()
{
Expand Down Expand Up @@ -3105,7 +3108,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 to be zero 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) zeros found in shrink. Therefore, they are very likely
// to be in-memory, and seldomly do we need to look them up in disk.
HaoranYi marked this conversation as resolved.
Show resolved Hide resolved
candidates_bin.insert(
removed_pubkey,
CleaningInfo {
can_contain_zero: true,
..Default::default()
},
);
}
}
}
Expand All @@ -3119,12 +3133,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 +3170,26 @@ impl AccountsDb {
std::iter::repeat_with(|| RwLock::new(HashMap::<Pubkey, CleaningInfo>::new()))
.take(num_bins)
.collect();

let insert_candidate = |pubkey, is_zero| {
HaoranYi marked this conversation as resolved.
Show resolved Hide resolved
let index = self.accounts_index.bin_calculator.bin_from_pubkey(&pubkey);
let mut candidates_bin = candidates[index].write().unwrap();

match candidates_bin.entry(pubkey) {
Entry::Occupied(occupied) => {
if is_zero {
occupied.into_mut().can_contain_zero = true;
}
}
Entry::Vacant(vacant) => {
vacant.insert(CleaningInfo {
can_contain_zero: is_zero,
..Default::default()
});
}
}
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved
};

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 +3202,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 = index.index_info.lamports == 0;
HaoranYi marked this conversation as resolved.
Show resolved Hide resolved
insert_candidate(pubkey, is_zero);
});
});
oldest_dirty_slot
})
Expand Down Expand Up @@ -3226,7 +3257,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 +3473,11 @@ impl AccountsDb {
},
None,
false,
ScanFilter::All,
if candidate_info.can_contain_zero {
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 +3528,7 @@ impl AccountsDb {
CleaningInfo {
slot_list,
ref_count,
..
},
) in candidates_bin.write().unwrap().iter_mut()
{
Expand Down Expand Up @@ -3572,6 +3608,7 @@ impl AccountsDb {
let CleaningInfo {
slot_list,
ref_count: _,
..
} = cleaning_info;
(!slot_list.is_empty()).then_some((
*pubkey,
Expand Down Expand Up @@ -3875,6 +3912,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 +12997,7 @@ pub mod tests {
CleaningInfo {
slot_list: rooted_entries,
ref_count,
..Default::default()
},
);
}
Expand All @@ -12969,6 +13008,7 @@ pub mod tests {
CleaningInfo {
slot_list: list,
ref_count,
..
},
) in candidates_bin.iter()
{
Expand Down Expand Up @@ -15221,6 +15261,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