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

Change read-only-cache to be keyed on pubkey only #576

Merged
merged 2 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion accounts-db/src/accounts_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6437,7 +6437,7 @@ impl AccountsDb {
// based on the patterns of how a validator writes accounts, it is almost always the case that there is no read only cache entry
// for this pubkey and slot. So, we can give that hint to the `remove` for performance.
self.read_only_accounts_cache
.remove_assume_not_present(*pubkey, slot);
.remove_assume_not_present(*pubkey);
});
}
calc_stored_meta_time.stop();
Expand Down
115 changes: 65 additions & 50 deletions accounts-db/src/read_only_accounts_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,16 @@ use {
const CACHE_ENTRY_SIZE: usize =
std::mem::size_of::<ReadOnlyAccountCacheEntry>() + 2 * std::mem::size_of::<ReadOnlyCacheKey>();

type ReadOnlyCacheKey = (Pubkey, Slot);
type ReadOnlyCacheKey = Pubkey;

#[derive(Debug)]
struct ReadOnlyAccountCacheEntry {
account: AccountSharedData,
/// 'slot' tracks when the 'account' is stored. This important for
/// correctness. When 'loading' from the cache by pubkey+slot, we need to
/// make sure that both pubkey and slot matches in the cache. Otherwise, we
/// may return the wrong account.
slot: Slot,
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved
/// Index of the entry in the eviction queue.
index: AtomicU32,
/// lower bits of last timestamp when eviction queue was updated, in ms
Expand Down Expand Up @@ -142,33 +147,42 @@ impl ReadOnlyAccountsCache {

/// true if pubkey is in cache at slot
pub(crate) fn in_cache(&self, pubkey: &Pubkey, slot: Slot) -> bool {
self.cache.contains_key(&(*pubkey, slot))
if let Some(entry) = self.cache.get(pubkey) {
entry.slot == slot
} else {
false
}
}

pub(crate) fn load(&self, pubkey: Pubkey, slot: Slot) -> Option<AccountSharedData> {
let (account, load_us) = measure_us!({
let key = (pubkey, slot);
let Some(entry) = self.cache.get(&key) else {
let mut found = None;
if let Some(entry) = self.cache.get(&pubkey) {
if entry.slot == slot {
// Move the entry to the end of the queue.
// self.queue is modified while holding a reference to the cache entry;
// so that another thread cannot write to the same key.
// If we updated the eviction queue within this much time, then leave it where it is. We're likely to hit it again.
let update_lru = entry.ms_since_last_update() >= self.ms_to_skip_lru_update;
if update_lru {
let mut queue = self.queue.lock().unwrap();
queue.remove(entry.index());
entry.set_index(queue.insert_last(pubkey));
entry
.last_update_time
.store(ReadOnlyAccountCacheEntry::timestamp(), Ordering::Release);
}
let account = entry.account.clone();
drop(entry);
self.stats.hits.fetch_add(1, Ordering::Relaxed);
found = Some(account);
}
}

if found.is_none() {
self.stats.misses.fetch_add(1, Ordering::Relaxed);
return None;
};
// Move the entry to the end of the queue.
// self.queue is modified while holding a reference to the cache entry;
// so that another thread cannot write to the same key.
// If we updated the eviction queue within this much time, then leave it where it is. We're likely to hit it again.
let update_lru = entry.ms_since_last_update() >= self.ms_to_skip_lru_update;
if update_lru {
let mut queue = self.queue.lock().unwrap();
queue.remove(entry.index());
entry.set_index(queue.insert_last(key));
entry
.last_update_time
.store(ReadOnlyAccountCacheEntry::timestamp(), Ordering::Release);
}
let account = entry.account.clone();
drop(entry);
self.stats.hits.fetch_add(1, Ordering::Relaxed);
Some(account)
found
});
self.stats.load_us.fetch_add(load_us, Ordering::Relaxed);
account
Expand All @@ -181,27 +195,27 @@ impl ReadOnlyAccountsCache {
pub(crate) fn store(&self, pubkey: Pubkey, slot: Slot, account: AccountSharedData) {
let measure_store = Measure::start("");
self.highest_slot_stored.fetch_max(slot, Ordering::Release);
let key = (pubkey, slot);
let account_size = Self::account_size(&account);
self.data_size.fetch_add(account_size, Ordering::Relaxed);
// self.queue is modified while holding a reference to the cache entry;
// so that another thread cannot write to the same key.
match self.cache.entry(key) {
match self.cache.entry(pubkey) {
Entry::Vacant(entry) => {
// Insert the entry at the end of the queue.
let mut queue = self.queue.lock().unwrap();
let index = queue.insert_last(key);
entry.insert(ReadOnlyAccountCacheEntry::new(account, index));
let index = queue.insert_last(pubkey);
entry.insert(ReadOnlyAccountCacheEntry::new(account, slot, index));
}
Entry::Occupied(mut entry) => {
let entry = entry.get_mut();
let account_size = Self::account_size(&entry.account);
self.data_size.fetch_sub(account_size, Ordering::Relaxed);
entry.account = account;
entry.slot = slot;
// Move the entry to the end of the queue.
let mut queue = self.queue.lock().unwrap();
queue.remove(entry.index());
entry.set_index(queue.insert_last(key));
entry.set_index(queue.insert_last(pubkey));
}
};

Expand All @@ -219,18 +233,14 @@ impl ReadOnlyAccountsCache {

/// remove entry if it exists.
/// Assume the entry does not exist for performance.
pub(crate) fn remove_assume_not_present(
&self,
pubkey: Pubkey,
slot: Slot,
) -> Option<AccountSharedData> {
pub(crate) fn remove_assume_not_present(&self, pubkey: Pubkey) -> Option<AccountSharedData> {
// get read lock first to see if the entry exists
_ = self.cache.get(&(pubkey, slot))?;
self.remove(pubkey, slot)
_ = self.cache.get(&pubkey)?;
self.remove(pubkey)
}

pub(crate) fn remove(&self, pubkey: Pubkey, slot: Slot) -> Option<AccountSharedData> {
Self::do_remove(&(pubkey, slot), &self.cache, &self.queue, &self.data_size)
pub(crate) fn remove(&self, pubkey: Pubkey) -> Option<AccountSharedData> {
Self::do_remove(&pubkey, &self.cache, &self.queue, &self.data_size)
}

/// Removes `key` from the cache, if present, and returns the removed account
Expand Down Expand Up @@ -338,11 +348,12 @@ impl ReadOnlyAccountsCache {
}

impl ReadOnlyAccountCacheEntry {
fn new(account: AccountSharedData, index: Index) -> Self {
fn new(account: AccountSharedData, slot: Slot, index: Index) -> Self {
let index = unsafe { std::mem::transmute::<Index, u32>(index) };
let index = AtomicU32::new(index);
Self {
account,
slot,
index,
last_update_time: AtomicU32::new(Self::timestamp()),
}
Expand Down Expand Up @@ -423,7 +434,7 @@ mod tests {
assert!(cache.load(Pubkey::default(), slot).is_none());
assert_eq!(0, cache.cache_len());
assert_eq!(0, cache.data_size());
cache.remove(Pubkey::default(), slot); // assert no panic
cache.remove(Pubkey::default()); // assert no panic
let key1 = Pubkey::new_unique();
let key2 = Pubkey::new_unique();
let key3 = Pubkey::new_unique();
Expand All @@ -439,6 +450,10 @@ mod tests {
cache.evict_in_foreground();
assert_eq!(100 + per_account_size, cache.data_size());
assert!(accounts_equal(&cache.load(key1, slot).unwrap(), &account1));
// pass a wrong slot and check that load fails
assert!(cache.load(key1, slot + 1).is_none());
Copy link
Author

@HaoranYi HaoranYi Apr 10, 2024

Choose a reason for hiding this comment

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

  1. calling load correctly checks slot?
    Added coverage.

// insert another entry for slot+1, and assert only one entry for key1 is in the cache
cache.store(key1, slot + 1, account1.clone());
Copy link
Author

Choose a reason for hiding this comment

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

  1. storing in a new slot "removes" the old one?
    Added coverage.

assert_eq!(1, cache.cache_len());
cache.store(key2, slot, account2.clone());
cache.evict_in_foreground();
Expand All @@ -450,7 +465,7 @@ mod tests {
assert_eq!(100 + per_account_size, cache.data_size());
assert!(accounts_equal(&cache.load(key2, slot).unwrap(), &account1));
assert_eq!(1, cache.cache_len());
cache.remove(key2, slot);
cache.remove(key2);
assert_eq!(0, cache.data_size());
assert_eq!(0, cache.cache_len());

Expand Down Expand Up @@ -507,16 +522,18 @@ mod tests {
rng.fill(&mut arr[..]);
Pubkey::new_from_array(arr)
})
.take(7)
.take(35)
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved
.collect();
let mut hash_map = HashMap::<ReadOnlyCacheKey, (AccountSharedData, usize)>::new();
let mut hash_map = HashMap::<ReadOnlyCacheKey, (AccountSharedData, Slot, usize)>::new();
for ix in 0..1000 {
if rng.gen_bool(0.1) {
let key = *cache.cache.iter().choose(&mut rng).unwrap().key();
let (pubkey, slot) = key;
let account = cache.load(pubkey, slot).unwrap();
let (other, index) = hash_map.get_mut(&key).unwrap();
let element = cache.cache.iter().choose(&mut rng).unwrap();
let (pubkey, entry) = element.pair();
let slot = entry.slot;
let account = cache.load(*pubkey, slot).unwrap();
let (other, other_slot, index) = hash_map.get_mut(pubkey).unwrap();
assert_eq!(account, *other);
assert_eq!(slot, *other_slot);
*index = ix;
} else {
let mut data = vec![0u8; DATA_SIZE];
Expand All @@ -530,8 +547,7 @@ mod tests {
});
let slot = *slots.choose(&mut rng).unwrap();
let pubkey = *pubkeys.choose(&mut rng).unwrap();
let key = (pubkey, slot);
hash_map.insert(key, (account.clone(), ix));
hash_map.insert(pubkey, (account.clone(), slot, ix));
cache.store(pubkey, slot, account);
cache.evict_in_foreground();
}
Expand All @@ -541,11 +557,10 @@ mod tests {
let index = hash_map
.iter()
.filter(|(k, _)| cache.cache.contains_key(k))
.map(|(_, (_, ix))| *ix)
.map(|(_, (_, _, ix))| *ix)
.min()
.unwrap();
for (key, (account, ix)) in hash_map {
let (pubkey, slot) = key;
for (pubkey, (account, slot, ix)) in hash_map {
assert_eq!(
cache.load(pubkey, slot),
if ix < index { None } else { Some(account) }
Expand Down
Loading