From 387d9d9c77577b7489bda43bf906eeb5e30d0b84 Mon Sep 17 00:00:00 2001 From: Christian Kamm Date: Fri, 20 May 2022 12:03:36 +0200 Subject: [PATCH] Store missing unupdated executors earlier --- program-runtime/src/invoke_context.rs | 35 +++++++----- runtime/src/bank.rs | 79 +++++++++++++++++---------- 2 files changed, 72 insertions(+), 42 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index b7014233b..584d2ee65 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -77,13 +77,23 @@ pub trait Executor: Debug + Send + Sync { pub type Executors = HashMap; +#[repr(u8)] +#[derive(PartialEq, Debug)] +enum TransactionExecutorStatus { + /// Executor was already in the cache, no update needed + Cached, + /// Executor was missing from the cache, but not updated + Missing, + /// Executor is for an updated program + Updated, +} + /// Tracks whether a given executor is "dirty" and needs to updated in the /// executors cache #[derive(Debug)] pub struct TransactionExecutor { executor: Arc, - is_miss: bool, - is_updated: bool, + status: TransactionExecutorStatus, } impl TransactionExecutor { @@ -92,8 +102,7 @@ impl TransactionExecutor { pub fn new_cached(executor: Arc) -> Self { Self { executor, - is_miss: false, - is_updated: false, + status: TransactionExecutorStatus::Cached, } } @@ -102,8 +111,7 @@ impl TransactionExecutor { pub fn new_miss(executor: Arc) -> Self { Self { executor, - is_miss: true, - is_updated: false, + status: TransactionExecutorStatus::Missing, } } @@ -112,22 +120,21 @@ impl TransactionExecutor { pub fn new_updated(executor: Arc) -> Self { Self { executor, - is_miss: false, - is_updated: true, + status: TransactionExecutorStatus::Updated, } } - pub fn is_dirty(&self, include_updates: bool) -> bool { - self.is_miss || (include_updates && self.is_updated) + pub fn is_missing(&self) -> bool { + self.status == TransactionExecutorStatus::Missing + } + + pub fn is_updated(&self) -> bool { + self.status == TransactionExecutorStatus::Updated } pub fn get(&self) -> Arc { self.executor.clone() } - - pub fn clear_miss_for_test(&mut self) { - self.is_miss = false; - } } /// Compute meter diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 234383c67..aa100e79f 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4171,18 +4171,26 @@ impl Bank { Rc::new(RefCell::new(executors)) } - /// Add executors back to the bank's cache if modified - fn update_executors(&self, allow_updates: bool, executors: Rc>) { + /// Add executors back to the bank's cache if they were missing and not updated + fn store_missing_executors(&self, executors: &RefCell) { + self.store_executors_internal(executors, |e| e.is_missing()) + } + + /// Add updated executors back to the bank's cache + fn store_updated_executors(&self, executors: &RefCell) { + self.store_executors_internal(executors, |e| e.is_updated()) + } + + /// Helper to write a selection of executors to the bank's cache + fn store_executors_internal( + &self, + executors: &RefCell, + selector: impl Fn(&TransactionExecutor) -> bool, + ) { let executors = executors.borrow(); let dirty_executors: Vec<_> = executors .iter() - .filter_map(|(key, executor)| { - if executor.is_dirty(allow_updates) { - Some((key, executor.get())) - } else { - None - } - }) + .filter_map(|(key, executor)| selector(executor).then(|| (key, executor.get()))) .collect(); if !dirty_executors.is_empty() { @@ -4272,6 +4280,14 @@ impl Bank { process_message_time.as_us() ); + let mut store_missing_executors_time = Measure::start("store_missing_executors_time"); + self.store_missing_executors(&executors); + store_missing_executors_time.stop(); + saturating_add_assign!( + timings.execute_accessories.update_executors_us, + store_missing_executors_time.as_us() + ); + let status = process_result .and_then(|info| { let post_account_state_info = @@ -4891,16 +4907,18 @@ impl Bank { sanitized_txs.len() ); - let mut update_executors_time = Measure::start("update_executors_time"); + let mut store_updated_executors_time = Measure::start("store_updated_executors_time"); for execution_result in &execution_results { if let TransactionExecutionResult::Executed { details, executors } = execution_result { - self.update_executors(details.status.is_ok(), executors.clone()); + if details.status.is_ok() { + self.store_updated_executors(executors); + } } } - update_executors_time.stop(); + store_updated_executors_time.stop(); saturating_add_assign!( timings.execute_accessories.update_executors_us, - update_executors_time.as_us() + store_updated_executors_time.as_us() ); let accounts_data_len_delta = execution_results @@ -14593,12 +14611,8 @@ pub(crate) mod tests { executors.insert(key3, TransactionExecutor::new_cached(executor.clone())); executors.insert(key4, TransactionExecutor::new_cached(executor.clone())); let executors = Rc::new(RefCell::new(executors)); - executors - .borrow_mut() - .get_mut(&key1) - .unwrap() - .clear_miss_for_test(); - bank.update_executors(true, executors); + bank.store_missing_executors(&executors); + bank.store_updated_executors(&executors); let executors = bank.get_executors(accounts); assert_eq!(executors.borrow().len(), 0); @@ -14606,15 +14620,24 @@ pub(crate) mod tests { let mut executors = Executors::default(); executors.insert(key1, TransactionExecutor::new_miss(executor.clone())); executors.insert(key2, TransactionExecutor::new_miss(executor.clone())); - executors.insert(key3, TransactionExecutor::new_miss(executor.clone())); + executors.insert(key3, TransactionExecutor::new_updated(executor.clone())); executors.insert(key4, TransactionExecutor::new_miss(executor.clone())); let executors = Rc::new(RefCell::new(executors)); - bank.update_executors(true, executors); - let executors = bank.get_executors(accounts); - assert_eq!(executors.borrow().len(), 3); - assert!(executors.borrow().contains_key(&key1)); - assert!(executors.borrow().contains_key(&key2)); - assert!(executors.borrow().contains_key(&key3)); + + // store the new_miss + bank.store_missing_executors(&executors); + let stored_executors = bank.get_executors(accounts); + assert_eq!(stored_executors.borrow().len(), 2); + assert!(stored_executors.borrow().contains_key(&key1)); + assert!(stored_executors.borrow().contains_key(&key2)); + + // store the new_updated + bank.store_updated_executors(&executors); + let stored_executors = bank.get_executors(accounts); + assert_eq!(stored_executors.borrow().len(), 3); + assert!(stored_executors.borrow().contains_key(&key1)); + assert!(stored_executors.borrow().contains_key(&key2)); + assert!(stored_executors.borrow().contains_key(&key3)); // Check inheritance let bank = Bank::new_from_parent(&Arc::new(bank), &solana_sdk::pubkey::new_rand(), 1); @@ -14658,7 +14681,7 @@ pub(crate) mod tests { let mut executors = Executors::default(); executors.insert(key1, TransactionExecutor::new_miss(executor.clone())); let executors = Rc::new(RefCell::new(executors)); - root.update_executors(true, executors); + root.store_missing_executors(&executors); let executors = root.get_executors(accounts); assert_eq!(executors.borrow().len(), 1); @@ -14673,7 +14696,7 @@ pub(crate) mod tests { let mut executors = Executors::default(); executors.insert(key2, TransactionExecutor::new_miss(executor.clone())); let executors = Rc::new(RefCell::new(executors)); - fork1.update_executors(true, executors); + fork1.store_missing_executors(&executors); let executors = fork1.get_executors(accounts); assert_eq!(executors.borrow().len(), 2);