From af7f08eba464859cb0b0778ec31ab5f5c8abb8f3 Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Wed, 29 Jun 2022 11:45:53 +0000 Subject: [PATCH] uses OnceCell instead of RwLock+Once to cache vote-state in vote-account (#26257) RwLock seems excessive since only the very 1st call to VoteAccount::vote_state will write-lock the inner field. Future calls would also incur overhead of an RwLockReadGuard. once_cell::sync::OnceCell provides a matching api to the desired functionality. --- Cargo.lock | 6 +++-- frozen-abi/Cargo.toml | 1 + frozen-abi/src/abi_example.rs | 6 +++++ programs/bpf/Cargo.lock | 6 +++-- runtime/Cargo.toml | 1 + runtime/src/bank.rs | 21 +++++++++--------- runtime/src/stake_account.rs | 10 ++++----- runtime/src/stakes.rs | 15 ------------- runtime/src/vote_account.rs | 41 ++++++++++------------------------- 9 files changed, 42 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 578b9abf60..df37853f86 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2820,9 +2820,9 @@ checksum = "830b246a0e5f20af87141b25c173cd1b609bd7779a4617d6ec582abaf90870f3" [[package]] name = "once_cell" -version = "1.8.0" +version = "1.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "692fcb63b64b1758029e0a96ee63e049ce8c5948587f2f7208df04625e5f6b56" +checksum = "7709cef83f0c1f58f666e746a08b21e0085f7440fa6a29cc194d68aac97a4225" [[package]] name = "opaque-debug" @@ -5113,6 +5113,7 @@ dependencies = [ "lazy_static", "log", "memmap2", + "once_cell", "rustc_version 0.4.0", "serde", "serde_bytes", @@ -5876,6 +5877,7 @@ dependencies = [ "num-derive", "num-traits", "num_cpus", + "once_cell", "ouroboros", "rand 0.7.3", "rand_chacha 0.2.2", diff --git a/frozen-abi/Cargo.toml b/frozen-abi/Cargo.toml index 87a7b4be90..b2a9f191e1 100644 --- a/frozen-abi/Cargo.toml +++ b/frozen-abi/Cargo.toml @@ -14,6 +14,7 @@ bs58 = "0.4.0" bv = { version = "0.11.1", features = ["serde"] } lazy_static = "1.4.0" log = "0.4.17" +once_cell = "1.12.0" serde = "1.0.137" serde_bytes = "0.11" serde_derive = "1.0.103" diff --git a/frozen-abi/src/abi_example.rs b/frozen-abi/src/abi_example.rs index 5e0749e5aa..e0dfa50b8a 100644 --- a/frozen-abi/src/abi_example.rs +++ b/frozen-abi/src/abi_example.rs @@ -555,3 +555,9 @@ impl AbiEnumVisitor for Result { digester.create_child() } } + +impl AbiExample for once_cell::sync::OnceCell { + fn example() -> Self { + Self::with_value(T::example()) + } +} diff --git a/programs/bpf/Cargo.lock b/programs/bpf/Cargo.lock index 5cb075d714..5741ae01d1 100644 --- a/programs/bpf/Cargo.lock +++ b/programs/bpf/Cargo.lock @@ -2548,9 +2548,9 @@ checksum = "830b246a0e5f20af87141b25c173cd1b609bd7779a4617d6ec582abaf90870f3" [[package]] name = "once_cell" -version = "1.8.0" +version = "1.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "692fcb63b64b1758029e0a96ee63e049ce8c5948587f2f7208df04625e5f6b56" +checksum = "7709cef83f0c1f58f666e746a08b21e0085f7440fa6a29cc194d68aac97a4225" [[package]] name = "opaque-debug" @@ -4703,6 +4703,7 @@ dependencies = [ "lazy_static", "log", "memmap2", + "once_cell", "rustc_version", "serde", "serde_bytes", @@ -5210,6 +5211,7 @@ dependencies = [ "num-derive", "num-traits", "num_cpus", + "once_cell", "ouroboros", "rand 0.7.3", "rayon", diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index fa907078c9..cacaa531be 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -32,6 +32,7 @@ memmap2 = "0.5.3" num-derive = { version = "0.3" } num-traits = { version = "0.2" } num_cpus = "1.13.1" +once_cell = "1.12.0" ouroboros = "0.15.0" rand = "0.7.0" rayon = "1.5.3" diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index ac770c16fc..4606d26a72 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2724,11 +2724,15 @@ impl Bank { "distributed inflation: {} (rounded from: {})", validator_rewards_paid, validator_rewards ); - - let num_stake_accounts = self.stakes_cache.num_stake_accounts(); - let num_vote_accounts = self.stakes_cache.num_vote_accounts(); - let num_staked_nodes = self.stakes_cache.num_staked_nodes(); - + // TODO: staked_nodes forces an eager stakes calculation. remove it! + let (num_stake_accounts, num_vote_accounts, num_staked_nodes) = { + let stakes = self.stakes_cache.stakes(); + ( + stakes.stake_delegations().len(), + stakes.vote_accounts().len(), + stakes.staked_nodes().len(), + ) + }; self.capitalization .fetch_add(validator_rewards_paid, Relaxed); @@ -2817,6 +2821,7 @@ impl Bank { } }; if cached_stake_account != &stake_account { + invalid_cached_stake_accounts.fetch_add(1, Relaxed); let mut cached_account = cached_stake_account.account().clone(); // We could have collected rent on the loaded account already in this new epoch (we could be at partition_index 12, for example). // So, we may need to adjust the rent_epoch of the cached account. So, update rent_epoch and compare just the accounts. @@ -2829,8 +2834,6 @@ impl Bank { stake_pubkey, &self.rewrites_skipped_this_slot, ); - // prior behavior was to always count any account that mismatched for any reason - invalid_cached_stake_accounts.fetch_add(1, Relaxed); if &cached_account != stake_account.account() { info!( "cached stake account mismatch: {}: {:?}, {:?}", @@ -5051,9 +5054,7 @@ impl Bank { None } else { total_staked += *staked; - account - .node_pubkey() - .map(|node_pubkey| (node_pubkey, *staked)) + Some((account.node_pubkey()?, *staked)) } }) .collect::>(); diff --git a/runtime/src/stake_account.rs b/runtime/src/stake_account.rs index e13b16efa4..b335255d7e 100644 --- a/runtime/src/stake_account.rs +++ b/runtime/src/stake_account.rs @@ -44,6 +44,10 @@ impl StakeAccount { pub(crate) fn stake_state(&self) -> &StakeState { &self.stake_state } + + pub(crate) fn account(&self) -> &AccountSharedData { + &self.account + } } impl StakeAccount { @@ -103,12 +107,6 @@ impl From> for (AccountSharedData, StakeState) { } } -impl StakeAccount { - pub fn account(&self) -> &AccountSharedData { - &self.account - } -} - impl PartialEq> for StakeAccount { fn eq(&self, other: &StakeAccount) -> bool { let StakeAccount { diff --git a/runtime/src/stakes.rs b/runtime/src/stakes.rs index cb57b28ee9..2497d716e5 100644 --- a/runtime/src/stakes.rs +++ b/runtime/src/stakes.rs @@ -57,21 +57,6 @@ impl StakesCache { self.0.read().unwrap() } - pub fn num_stake_accounts(&self) -> usize { - let stakes = self.0.read().unwrap(); - stakes.stake_delegations.len() - } - - pub fn num_vote_accounts(&self) -> usize { - let stakes = self.0.read().unwrap(); - stakes.vote_accounts.num_vote_accounts() - } - - pub fn num_staked_nodes(&self) -> usize { - let stakes = self.0.read().unwrap(); - stakes.vote_accounts.num_staked_nodes() - } - pub fn check_and_store(&self, pubkey: &Pubkey, account: &impl ReadableAccount) { // TODO: If the account is already cached as a vote or stake account // but the owner changes, then this needs to evict the account from diff --git a/runtime/src/vote_account.rs b/runtime/src/vote_account.rs index 3aaf830f17..d7324ab8db 100644 --- a/runtime/src/vote_account.rs +++ b/runtime/src/vote_account.rs @@ -1,5 +1,6 @@ use { itertools::Itertools, + once_cell::sync::OnceCell, serde::ser::{Serialize, Serializer}, solana_sdk::{ account::{accounts_equal, AccountSharedData, ReadableAccount}, @@ -11,17 +12,11 @@ use { cmp::Ordering, collections::{hash_map::Entry, HashMap}, iter::FromIterator, - sync::{Arc, Once, RwLock, RwLockReadGuard}, + sync::{Arc, Once, RwLock}, }, thiserror::Error, }; -// The value here does not matter. It will be overwritten -// at the first call to VoteAccount::vote_state(). -const INVALID_VOTE_STATE: Result = Err(Error::InstructionError( - InstructionError::InvalidAccountData, -)); - #[derive(Clone, Debug, PartialEq, AbiExample, Deserialize)] #[serde(try_from = "AccountSharedData")] pub struct VoteAccount(Arc); @@ -37,8 +32,7 @@ pub enum Error { #[derive(Debug, AbiExample)] struct VoteAccountInner { account: AccountSharedData, - vote_state: RwLock>, - vote_state_once: Once, + vote_state: OnceCell>, } pub type VoteAccountsHashMap = HashMap; @@ -60,16 +54,6 @@ pub struct VoteAccounts { staked_nodes_once: Once, } -impl VoteAccounts { - pub fn num_vote_accounts(&self) -> usize { - self.vote_accounts.len() - } - - pub fn num_staked_nodes(&self) -> usize { - self.staked_nodes().len() - } -} - impl VoteAccount { pub(crate) fn lamports(&self) -> u64 { self.0.account.lamports() @@ -79,17 +63,16 @@ impl VoteAccount { self.0.account.owner() } - pub fn vote_state(&self) -> RwLockReadGuard> { - let inner = &self.0; - inner.vote_state_once.call_once(|| { - let vote_state = VoteState::deserialize(inner.account.data()); - *inner.vote_state.write().unwrap() = vote_state.map_err(Error::from); - }); - inner.vote_state.read().unwrap() + pub fn vote_state(&self) -> &Result { + // VoteState::deserialize deserializes a VoteStateVersions and then + // calls VoteStateVersions::convert_to_current. + self.0 + .vote_state + .get_or_init(|| VoteState::deserialize(self.0.account.data()).map_err(Error::from)) } pub(crate) fn is_deserialized(&self) -> bool { - self.0.vote_state_once.is_completed() + self.0.vote_state.get().is_some() } /// VoteState.node_pubkey of this vote-account. @@ -252,8 +235,7 @@ impl TryFrom for VoteAccountInner { } Ok(Self { account, - vote_state: RwLock::new(INVALID_VOTE_STATE), - vote_state_once: Once::new(), + vote_state: OnceCell::new(), }) } } @@ -263,7 +245,6 @@ impl PartialEq for VoteAccountInner { let Self { account, vote_state: _, - vote_state_once: _, } = self; account == &other.account }