From 1ad1164e1cb5b2bfd4ba8a17629debcc50073187 Mon Sep 17 00:00:00 2001 From: jeff washington Date: Thu, 28 Mar 2024 17:28:01 -0500 Subject: [PATCH 1/6] dsiable read cache while populating stakes cache on load --- accounts-db/src/accounts_db.rs | 20 +++++++++++++++++++- runtime/src/bank.rs | 11 +++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index 0aabae8e93ea1b..c093e1d65c3444 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -1274,6 +1274,9 @@ pub struct AccountsDb { /// Keeps tracks of index into AppendVec on a per slot basis pub accounts_index: AccountInfoAccountsIndex, + /// if != 0, read cache is not updated on loads + disable_read_cache_updates: AtomicU64, + /// Some(offset) iff we want to squash old append vecs together into 'ancient append vecs' /// Some(offset) means for slots up to (max_slot - (slots_per_epoch - 'offset')), put them in ancient append vecs pub ancient_append_vec_offset: Option, @@ -2327,6 +2330,7 @@ impl AccountsDb { active_stats: ActiveStats::default(), skip_initial_hash_calc: false, ancient_append_vec_offset: None, + disable_read_cache_updates: AtomicU64::default(), accounts_index, storage: AccountStorage::default(), accounts_cache: AccountsCache::default(), @@ -5348,7 +5352,7 @@ impl AccountsDb { return None; } - if !is_cached { + if !is_cached && self.disable_read_cache_updates.load(Ordering::Relaxed) == 0 { /* We show this store into the read-only cache for account 'A' and future loads of 'A' from the read-only cache are safe/reflect 'A''s latest state on this fork. @@ -8348,6 +8352,20 @@ impl AccountsDb { } } + /// pass true to cause loads to NOT populate the read cache. + /// Balance true calls with false calls. + /// By default, all loads will populate the read cache. + /// This cannot be incorrect, it is only a performance penalty at worst. + pub fn disable_read_cache_updates(&self, disable: bool) { + if disable { + self.disable_read_cache_updates + .fetch_add(1, Ordering::Relaxed); + } else { + self.disable_read_cache_updates + .fetch_sub(1, Ordering::Relaxed); + } + } + fn store_accounts_unfrozen<'a, T: ReadableAccount + Sync + ZeroLamport + 'a>( &self, accounts: impl StorableAccounts<'a, T>, diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index c8afb7406164ae..7e61985fd975ff 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -1823,6 +1823,13 @@ impl Bank { // from Stakes by reading the full account state from // accounts-db. Note that it is crucial that these accounts are loaded // at the right slot and match precisely with serialized Delegations. + // Note that we are disabling the read cache while we populate the stakes cache. + // The stakes accounts will not be expected to be loaded again. + // If we populate the read cache with these loads, then we'll just soon have to evict these. + bank_rc + .accounts + .accounts_db + .disable_read_cache_updates(true); let stakes = Stakes::new(&fields.stakes, |pubkey| { let (account, _slot) = bank_rc.accounts.load_with_fixed_root(&ancestors, pubkey)?; Some(account) @@ -1831,6 +1838,10 @@ impl Bank { "Stakes cache is inconsistent with accounts-db. This can indicate \ a corrupted snapshot or bugs in cached accounts or accounts-db.", ); + bank_rc + .accounts + .accounts_db + .disable_read_cache_updates(false); let stakes_accounts_load_duration = now.elapsed(); let mut bank = Self { skipped_rewrites: Mutex::default(), From cdf81bd7532584bb0dafe1c510012c51e9e648f0 Mon Sep 17 00:00:00 2001 From: jeff washington Date: Fri, 29 Mar 2024 10:33:54 -0500 Subject: [PATCH 2/6] use struct with drop as api --- accounts-db/src/accounts_db.rs | 50 ++++++++++++++++++++++------------ runtime/src/bank.rs | 28 ++++++++----------- 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index c093e1d65c3444..3cfd8d592c1e3d 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -1268,6 +1268,31 @@ struct RemoveUnrootedSlotsSynchronization { type AccountInfoAccountsIndex = AccountsIndex; +/// When an instance of this exists, the read cache is not updated for account loads. +/// When all instances are dropped, the read cache is populated for account loads. +/// By default, all loads will populate the read cache. +/// This cannot be incorrect, it is only a performance penalty at worst. +pub struct DisableReadCacheUpdates<'a> { + accounts_db: &'a AccountsDb, +} + +impl<'a> DisableReadCacheUpdates<'a> { + pub fn new(accounts_db: &'a AccountsDb) -> Self { + accounts_db + .disable_read_cache_updates_count + .fetch_add(1, Ordering::Relaxed); + Self { accounts_db } + } +} + +impl<'a> Drop for DisableReadCacheUpdates<'a> { + fn drop(&mut self) { + self.accounts_db + .disable_read_cache_updates_count + .fetch_sub(1, Ordering::Relaxed); + } +} + // This structure handles the load/store of the accounts #[derive(Debug)] pub struct AccountsDb { @@ -1275,7 +1300,7 @@ pub struct AccountsDb { pub accounts_index: AccountInfoAccountsIndex, /// if != 0, read cache is not updated on loads - disable_read_cache_updates: AtomicU64, + disable_read_cache_updates_count: AtomicU64, /// Some(offset) iff we want to squash old append vecs together into 'ancient append vecs' /// Some(offset) means for slots up to (max_slot - (slots_per_epoch - 'offset')), put them in ancient append vecs @@ -2330,7 +2355,7 @@ impl AccountsDb { active_stats: ActiveStats::default(), skip_initial_hash_calc: false, ancient_append_vec_offset: None, - disable_read_cache_updates: AtomicU64::default(), + disable_read_cache_updates_count: AtomicU64::default(), accounts_index, storage: AccountStorage::default(), accounts_cache: AccountsCache::default(), @@ -5352,7 +5377,12 @@ impl AccountsDb { return None; } - if !is_cached && self.disable_read_cache_updates.load(Ordering::Relaxed) == 0 { + if !is_cached + && self + .disable_read_cache_updates_count + .load(Ordering::Relaxed) + == 0 + { /* We show this store into the read-only cache for account 'A' and future loads of 'A' from the read-only cache are safe/reflect 'A''s latest state on this fork. @@ -8352,20 +8382,6 @@ impl AccountsDb { } } - /// pass true to cause loads to NOT populate the read cache. - /// Balance true calls with false calls. - /// By default, all loads will populate the read cache. - /// This cannot be incorrect, it is only a performance penalty at worst. - pub fn disable_read_cache_updates(&self, disable: bool) { - if disable { - self.disable_read_cache_updates - .fetch_add(1, Ordering::Relaxed); - } else { - self.disable_read_cache_updates - .fetch_sub(1, Ordering::Relaxed); - } - } - fn store_accounts_unfrozen<'a, T: ReadableAccount + Sync + ZeroLamport + 'a>( &self, accounts: impl StorableAccounts<'a, T>, diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 7e61985fd975ff..1016378a14fd48 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -72,7 +72,8 @@ use { accounts::{AccountAddressFilter, Accounts, PubkeyAccountSlot}, accounts_db::{ AccountShrinkThreshold, AccountStorageEntry, AccountsDb, AccountsDbConfig, - CalcAccountsHashDataSource, VerifyAccountsHashAndLamportsConfig, + CalcAccountsHashDataSource, DisableReadCacheUpdates, + VerifyAccountsHashAndLamportsConfig, }, accounts_hash::{ AccountHash, AccountsHash, CalcAccountsHashConfig, HashStats, IncrementalAccountsHash, @@ -1826,22 +1827,17 @@ impl Bank { // Note that we are disabling the read cache while we populate the stakes cache. // The stakes accounts will not be expected to be loaded again. // If we populate the read cache with these loads, then we'll just soon have to evict these. - bank_rc - .accounts - .accounts_db - .disable_read_cache_updates(true); - let stakes = Stakes::new(&fields.stakes, |pubkey| { - let (account, _slot) = bank_rc.accounts.load_with_fixed_root(&ancestors, pubkey)?; - Some(account) - }) - .expect( - "Stakes cache is inconsistent with accounts-db. This can indicate \ + let stakes = { + let _guard = DisableReadCacheUpdates::new(&bank_rc.accounts.accounts_db); + Stakes::new(&fields.stakes, |pubkey| { + let (account, _slot) = bank_rc.accounts.load_with_fixed_root(&ancestors, pubkey)?; + Some(account) + }) + .expect( + "Stakes cache is inconsistent with accounts-db. This can indicate \ a corrupted snapshot or bugs in cached accounts or accounts-db.", - ); - bank_rc - .accounts - .accounts_db - .disable_read_cache_updates(false); + ) + }; let stakes_accounts_load_duration = now.elapsed(); let mut bank = Self { skipped_rewrites: Mutex::default(), From 1182d2002731bb7eacc1e7223b95c178c3274cc2 Mon Sep 17 00:00:00 2001 From: jeff washington Date: Fri, 29 Mar 2024 11:52:16 -0500 Subject: [PATCH 3/6] use LoadHint --- accounts-db/src/accounts.rs | 8 ++++++++ accounts-db/src/accounts_db.rs | 13 +++++-------- runtime/src/bank.rs | 11 +++++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 33a57d56461c78..d9fce64e36b12f 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -174,6 +174,14 @@ impl Accounts { self.load_slow(ancestors, pubkey, LoadHint::FixedMaxRoot) } + pub fn load_with_fixed_root_do_not_populate_read_cache( + &self, + ancestors: &Ancestors, + pubkey: &Pubkey, + ) -> Option<(AccountSharedData, Slot)> { + self.load_slow(ancestors, pubkey, LoadHint::FixedMaxRootDoNotPopulateReadCache) + } + pub fn load_without_fixed_root( &self, ancestors: &Ancestors, diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index 3cfd8d592c1e3d..41fef71b3cbece 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -787,6 +787,8 @@ pub enum LoadHint { // account loading, while maintaining the determinism of account loading and resultant // transaction execution thereof. FixedMaxRoot, + /// same as `FixedMaxRoot`, except do not populate the read cache on load + FixedMaxRootDoNotPopulateReadCache, // Caller can't hint the above safety assumption. Generally RPC and miscellaneous // other call-site falls into this category. The likelihood of slower path is slightly // increased as well. @@ -5148,7 +5150,7 @@ impl AccountsDb { // so retry. This works because in accounts cache flush, an account is written to // storage *before* it is removed from the cache match load_hint { - LoadHint::FixedMaxRoot => { + LoadHint::FixedMaxRootDoNotPopulateReadCache | LoadHint::FixedMaxRoot => { // it's impossible for this to fail for transaction loads from // replaying/banking more than once. // This is because: @@ -5168,7 +5170,7 @@ impl AccountsDb { } LoadedAccountAccessor::Stored(None) => { match load_hint { - LoadHint::FixedMaxRoot => { + LoadHint::FixedMaxRootDoNotPopulateReadCache | LoadHint::FixedMaxRoot => { // When running replay on the validator, or banking stage on the leader, // it should be very rare that the storage entry doesn't exist if the // entry in the accounts index is the latest version of this account. @@ -5377,12 +5379,7 @@ impl AccountsDb { return None; } - if !is_cached - && self - .disable_read_cache_updates_count - .load(Ordering::Relaxed) - == 0 - { + if !is_cached && load_hint != LoadHint::FixedMaxRootDoNotPopulateReadCache { /* We show this store into the read-only cache for account 'A' and future loads of 'A' from the read-only cache are safe/reflect 'A''s latest state on this fork. diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 1016378a14fd48..64d0b911edf49d 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -1827,17 +1827,16 @@ impl Bank { // Note that we are disabling the read cache while we populate the stakes cache. // The stakes accounts will not be expected to be loaded again. // If we populate the read cache with these loads, then we'll just soon have to evict these. - let stakes = { - let _guard = DisableReadCacheUpdates::new(&bank_rc.accounts.accounts_db); - Stakes::new(&fields.stakes, |pubkey| { - let (account, _slot) = bank_rc.accounts.load_with_fixed_root(&ancestors, pubkey)?; + let stakes = Stakes::new(&fields.stakes, |pubkey| { + let (account, _slot) = bank_rc + .accounts + .load_with_fixed_root_do_not_populate_read_cache(&ancestors, pubkey)?; Some(account) }) .expect( "Stakes cache is inconsistent with accounts-db. This can indicate \ a corrupted snapshot or bugs in cached accounts or accounts-db.", - ) - }; + ); let stakes_accounts_load_duration = now.elapsed(); let mut bank = Self { skipped_rewrites: Mutex::default(), From 80fa03348128558516f901596fb475edcbd59a6f Mon Sep 17 00:00:00 2001 From: jeff washington Date: Fri, 29 Mar 2024 11:53:08 -0500 Subject: [PATCH 4/6] remove disable_read_cache_updates_count --- accounts-db/src/accounts_db.rs | 29 ----------------------------- runtime/src/bank.rs | 11 +++++------ 2 files changed, 5 insertions(+), 35 deletions(-) diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index 41fef71b3cbece..f9d40da83e9f7c 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -1270,40 +1270,12 @@ struct RemoveUnrootedSlotsSynchronization { type AccountInfoAccountsIndex = AccountsIndex; -/// When an instance of this exists, the read cache is not updated for account loads. -/// When all instances are dropped, the read cache is populated for account loads. -/// By default, all loads will populate the read cache. -/// This cannot be incorrect, it is only a performance penalty at worst. -pub struct DisableReadCacheUpdates<'a> { - accounts_db: &'a AccountsDb, -} - -impl<'a> DisableReadCacheUpdates<'a> { - pub fn new(accounts_db: &'a AccountsDb) -> Self { - accounts_db - .disable_read_cache_updates_count - .fetch_add(1, Ordering::Relaxed); - Self { accounts_db } - } -} - -impl<'a> Drop for DisableReadCacheUpdates<'a> { - fn drop(&mut self) { - self.accounts_db - .disable_read_cache_updates_count - .fetch_sub(1, Ordering::Relaxed); - } -} - // This structure handles the load/store of the accounts #[derive(Debug)] pub struct AccountsDb { /// Keeps tracks of index into AppendVec on a per slot basis pub accounts_index: AccountInfoAccountsIndex, - /// if != 0, read cache is not updated on loads - disable_read_cache_updates_count: AtomicU64, - /// Some(offset) iff we want to squash old append vecs together into 'ancient append vecs' /// Some(offset) means for slots up to (max_slot - (slots_per_epoch - 'offset')), put them in ancient append vecs pub ancient_append_vec_offset: Option, @@ -2357,7 +2329,6 @@ impl AccountsDb { active_stats: ActiveStats::default(), skip_initial_hash_calc: false, ancient_append_vec_offset: None, - disable_read_cache_updates_count: AtomicU64::default(), accounts_index, storage: AccountStorage::default(), accounts_cache: AccountsCache::default(), diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 64d0b911edf49d..f9a785eb9b5a57 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -72,8 +72,7 @@ use { accounts::{AccountAddressFilter, Accounts, PubkeyAccountSlot}, accounts_db::{ AccountShrinkThreshold, AccountStorageEntry, AccountsDb, AccountsDbConfig, - CalcAccountsHashDataSource, DisableReadCacheUpdates, - VerifyAccountsHashAndLamportsConfig, + CalcAccountsHashDataSource, VerifyAccountsHashAndLamportsConfig, }, accounts_hash::{ AccountHash, AccountsHash, CalcAccountsHashConfig, HashStats, IncrementalAccountsHash, @@ -1831,10 +1830,10 @@ impl Bank { let (account, _slot) = bank_rc .accounts .load_with_fixed_root_do_not_populate_read_cache(&ancestors, pubkey)?; - Some(account) - }) - .expect( - "Stakes cache is inconsistent with accounts-db. This can indicate \ + Some(account) + }) + .expect( + "Stakes cache is inconsistent with accounts-db. This can indicate \ a corrupted snapshot or bugs in cached accounts or accounts-db.", ); let stakes_accounts_load_duration = now.elapsed(); From 187132ccbfa869a1fd0939f98e805a4a1355f3a1 Mon Sep 17 00:00:00 2001 From: jeff washington Date: Fri, 29 Mar 2024 11:54:25 -0500 Subject: [PATCH 5/6] add comment --- accounts-db/src/accounts.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index d9fce64e36b12f..3a44c4e288e2ae 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -174,6 +174,8 @@ impl Accounts { self.load_slow(ancestors, pubkey, LoadHint::FixedMaxRoot) } + /// same as `load_with_fixed_root` except: + /// if the account is not already in the read cache, it is NOT put in the read cache on successful load pub fn load_with_fixed_root_do_not_populate_read_cache( &self, ancestors: &Ancestors, From f566b045e330de02571a012085ce4b9519aca1ae Mon Sep 17 00:00:00 2001 From: jeff washington Date: Fri, 29 Mar 2024 13:34:32 -0500 Subject: [PATCH 6/6] fmt --- accounts-db/src/accounts.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 3a44c4e288e2ae..bfacc83b69a9e1 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -181,7 +181,11 @@ impl Accounts { ancestors: &Ancestors, pubkey: &Pubkey, ) -> Option<(AccountSharedData, Slot)> { - self.load_slow(ancestors, pubkey, LoadHint::FixedMaxRootDoNotPopulateReadCache) + self.load_slow( + ancestors, + pubkey, + LoadHint::FixedMaxRootDoNotPopulateReadCache, + ) } pub fn load_without_fixed_root(