diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index a559814881..a7185bd7a0 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -71,18 +71,57 @@ pub trait Executor: Debug + Send + Sync { ) -> Result<(), InstructionError>; } -#[derive(Default)] -pub struct Executors { - pub executors: HashMap>, - pub is_dirty: bool, +pub type Executors = HashMap; + +/// Tracks whether a given executor is "dirty" and needs to updated in the +/// executors cache +pub struct TransactionExecutor { + executor: Arc, + is_miss: bool, + is_updated: bool, } -impl Executors { - pub fn insert(&mut self, key: Pubkey, executor: Arc) { - let _ = self.executors.insert(key, executor); - self.is_dirty = true; + +impl TransactionExecutor { + /// Wraps an executor and tracks that it doesn't need to be updated in the + /// executors cache. + pub fn new_cached(executor: Arc) -> Self { + Self { + executor, + is_miss: false, + is_updated: false, + } } - pub fn get(&self, key: &Pubkey) -> Option> { - self.executors.get(key).cloned() + + /// Wraps an executor and tracks that it needs to be updated in the + /// executors cache. + pub fn new_miss(executor: Arc) -> Self { + Self { + executor, + is_miss: true, + is_updated: false, + } + } + + /// Wraps an executor and tracks that it needs to be updated in the + /// executors cache only if the transaction succeeded. + pub fn new_updated(executor: Arc) -> Self { + Self { + executor, + is_miss: false, + is_updated: true, + } + } + + pub fn is_dirty(&self, include_updates: bool) -> bool { + self.is_miss || (include_updates && self.is_updated) + } + + pub fn get(&self) -> Arc { + self.executor.clone() + } + + pub fn clear_miss_for_test(&mut self) { + self.is_miss = false; } } @@ -808,15 +847,26 @@ impl<'a> InvokeContext<'a> { &self.accounts_data_meter } - /// Loaders may need to do work in order to execute a program. Cache - /// the work that can be re-used across executions + /// Cache an executor that wasn't found in the cache pub fn add_executor(&self, pubkey: &Pubkey, executor: Arc) { - self.executors.borrow_mut().insert(*pubkey, executor); + self.executors + .borrow_mut() + .insert(*pubkey, TransactionExecutor::new_miss(executor)); + } + + /// Cache an executor that has changed + pub fn update_executor(&self, pubkey: &Pubkey, executor: Arc) { + self.executors + .borrow_mut() + .insert(*pubkey, TransactionExecutor::new_updated(executor)); } /// Get the completed loader work that can be re-used across execution pub fn get_executor(&self, pubkey: &Pubkey) -> Option> { - self.executors.borrow().get(pubkey) + self.executors + .borrow() + .get(pubkey) + .map(|tx_executor| tx_executor.executor.clone()) } /// Find an account_index and account by its key diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 5d4feb52f6..5bf484151c 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -510,7 +510,7 @@ fn process_loader_upgradeable_instruction( use_jit, true, )?; - invoke_context.add_executor(&new_program_id, executor); + invoke_context.update_executor(&new_program_id, executor); let keyed_accounts = invoke_context.get_keyed_accounts()?; let payer = keyed_account_at_index(keyed_accounts, first_instruction_account)?; @@ -658,7 +658,7 @@ fn process_loader_upgradeable_instruction( use_jit, true, )?; - invoke_context.add_executor(&new_program_id, executor); + invoke_context.update_executor(&new_program_id, executor); let keyed_accounts = invoke_context.get_keyed_accounts()?; let programdata = keyed_account_at_index(keyed_accounts, first_instruction_account)?; @@ -925,7 +925,7 @@ fn process_loader_instruction( create_executor(first_instruction_account, 0, invoke_context, use_jit, true)?; let keyed_accounts = invoke_context.get_keyed_accounts()?; let program = keyed_account_at_index(keyed_accounts, first_instruction_account)?; - invoke_context.add_executor(program.unsigned_key(), executor); + invoke_context.update_executor(program.unsigned_key(), executor); program.try_account_ref_mut()?.set_executable(true); ic_msg!( invoke_context, diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 375395c721..2fbd77ef80 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -79,7 +79,7 @@ use { instruction_recorder::InstructionRecorder, invoke_context::{ BuiltinProgram, Executor, Executors, ProcessInstructionWithContext, - TransactionAccountRefCells, + TransactionAccountRefCells, TransactionExecutor, }, log_collector::LogCollector, timings::ExecuteDetailsTimings, @@ -3546,32 +3546,40 @@ impl Bank { for key in message.account_keys_iter() { if let Some(executor) = cache.get(key) { - executors.insert(*key, executor); + executors.insert(*key, TransactionExecutor::new_cached(executor)); } } for program_indices_of_instruction in program_indices.iter() { for account_index in program_indices_of_instruction.iter() { let key = accounts[*account_index].0; if let Some(executor) = cache.get(&key) { - executors.insert(key, executor); + executors.insert(key, TransactionExecutor::new_cached(executor)); } } } - Rc::new(RefCell::new(Executors { - executors, - is_dirty: false, - })) + Rc::new(RefCell::new(executors)) } /// Add executors back to the bank's cache if modified - fn update_executors(&self, executors: Rc>) { + fn update_executors(&self, allow_updates: bool, executors: Rc>) { let executors = executors.borrow(); - if executors.is_dirty { + let dirty_executors: Vec<_> = executors + .iter() + .filter_map(|(key, executor)| { + if executor.is_dirty(allow_updates) { + Some((key, executor.get())) + } else { + None + } + }) + .collect(); + + if !dirty_executors.is_empty() { let mut cow_cache = self.cached_executors.write().unwrap(); let mut cache = cow_cache.write().unwrap(); - for (key, executor) in executors.executors.iter() { - cache.put(key, (*executor).clone()); + for (key, executor) in dirty_executors.into_iter() { + cache.put(key, executor); } } } @@ -3648,6 +3656,17 @@ impl Bank { self.load_accounts_data_len(), ); + self.update_executors(process_result.is_ok(), executors); + + let status = process_result + .map(|info| { + self.store_accounts_data_len(info.accounts_data_len); + }) + .map_err(|err| { + error_counters.instruction_error += 1; + err + }); + let log_messages: Option = log_collector.and_then(|log_collector| { Rc::try_unwrap(log_collector) @@ -3670,16 +3689,6 @@ impl Bank { return TransactionExecutionResult::NotExecuted(e); } - let status = process_result - .map(|info| { - self.store_accounts_data_len(info.accounts_data_len); - self.update_executors(executors); - }) - .map_err(|err| { - error_counters.instruction_error += 1; - err - }); - TransactionExecutionResult::Executed(TransactionExecutionDetails { status, log_messages, @@ -12891,50 +12900,54 @@ pub(crate) mod tests { // don't do any work if not dirty let mut executors = Executors::default(); - executors.insert(key1, executor.clone()); - executors.insert(key2, executor.clone()); - executors.insert(key3, executor.clone()); - executors.insert(key4, executor.clone()); + executors.insert(key1, TransactionExecutor::new_cached(executor.clone())); + executors.insert(key2, TransactionExecutor::new_cached(executor.clone())); + 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().is_dirty = false; - bank.update_executors(executors); + executors + .borrow_mut() + .get_mut(&key1) + .unwrap() + .clear_miss_for_test(); + bank.update_executors(true, executors); let executors = bank.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 0); + assert_eq!(executors.borrow().len(), 0); // do work let mut executors = Executors::default(); - executors.insert(key1, executor.clone()); - executors.insert(key2, executor.clone()); - executors.insert(key3, executor.clone()); - executors.insert(key4, executor.clone()); + 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(key4, TransactionExecutor::new_miss(executor.clone())); let executors = Rc::new(RefCell::new(executors)); - bank.update_executors(executors); + bank.update_executors(true, executors); let executors = bank.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 4); - assert!(executors.borrow().executors.contains_key(&key1)); - assert!(executors.borrow().executors.contains_key(&key2)); - assert!(executors.borrow().executors.contains_key(&key3)); - assert!(executors.borrow().executors.contains_key(&key4)); + assert_eq!(executors.borrow().len(), 4); + assert!(executors.borrow().contains_key(&key1)); + assert!(executors.borrow().contains_key(&key2)); + assert!(executors.borrow().contains_key(&key3)); + assert!(executors.borrow().contains_key(&key4)); // Check inheritance let bank = Bank::new_from_parent(&Arc::new(bank), &solana_sdk::pubkey::new_rand(), 1); let executors = bank.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 4); - assert!(executors.borrow().executors.contains_key(&key1)); - assert!(executors.borrow().executors.contains_key(&key2)); - assert!(executors.borrow().executors.contains_key(&key3)); - assert!(executors.borrow().executors.contains_key(&key4)); + assert_eq!(executors.borrow().len(), 4); + assert!(executors.borrow().contains_key(&key1)); + assert!(executors.borrow().contains_key(&key2)); + assert!(executors.borrow().contains_key(&key3)); + assert!(executors.borrow().contains_key(&key4)); bank.remove_executor(&key1); bank.remove_executor(&key2); bank.remove_executor(&key3); bank.remove_executor(&key4); let executors = bank.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 0); - assert!(!executors.borrow().executors.contains_key(&key1)); - assert!(!executors.borrow().executors.contains_key(&key2)); - assert!(!executors.borrow().executors.contains_key(&key3)); - assert!(!executors.borrow().executors.contains_key(&key4)); + assert_eq!(executors.borrow().len(), 0); + assert!(!executors.borrow().contains_key(&key1)); + assert!(!executors.borrow().contains_key(&key2)); + assert!(!executors.borrow().contains_key(&key3)); + assert!(!executors.borrow().contains_key(&key4)); } #[test] @@ -12958,36 +12971,36 @@ pub(crate) mod tests { // add one to root bank let mut executors = Executors::default(); - executors.insert(key1, executor.clone()); + executors.insert(key1, TransactionExecutor::new_miss(executor.clone())); let executors = Rc::new(RefCell::new(executors)); - root.update_executors(executors); + root.update_executors(true, executors); let executors = root.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().len(), 1); let fork1 = Bank::new_from_parent(&root, &Pubkey::default(), 1); let fork2 = Bank::new_from_parent(&root, &Pubkey::default(), 1); let executors = fork1.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().len(), 1); let executors = fork2.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().len(), 1); let mut executors = Executors::default(); - executors.insert(key2, executor.clone()); + executors.insert(key2, TransactionExecutor::new_miss(executor.clone())); let executors = Rc::new(RefCell::new(executors)); - fork1.update_executors(executors); + fork1.update_executors(true, executors); let executors = fork1.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 2); + assert_eq!(executors.borrow().len(), 2); let executors = fork2.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().len(), 1); fork1.remove_executor(&key1); let executors = fork1.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().len(), 1); let executors = fork2.get_executors(&message, accounts, program_indices); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().len(), 1); } #[test]