cache executors on failed transactions (backport #22308) (#22328)

* cache executors on failed transactions (#22308)

(cherry picked from commit 12e160269e)

# Conflicts:
#	program-runtime/src/invoke_context.rs
#	runtime/src/bank.rs

* resolve conflicts

Co-authored-by: Jack May <jack@solana.com>

program-runtime/src/invoke_context.rs

@@ -71,18 +71,57 @@ pub trait Executor: Debug + Send + Sync {
     ) -> Result<(), InstructionError>;
 }
 
-#[derive(Default)]
+pub type Executors = HashMap<Pubkey, TransactionExecutor>;
-pub struct Executors {
+
-    pub executors: HashMap<Pubkey, Arc<dyn Executor>>,
+/// Tracks whether a given executor is "dirty" and needs to updated in the
-    pub is_dirty: bool,
+/// executors cache
+pub struct TransactionExecutor {
+    executor: Arc<dyn Executor>,
+    is_miss: bool,
+    is_updated: bool,
 }
-impl Executors {
+
-    pub fn insert(&mut self, key: Pubkey, executor: Arc<dyn Executor>) {
+impl TransactionExecutor {
-        let _ = self.executors.insert(key, executor);
+    /// Wraps an executor and tracks that it doesn't need to be updated in the
-        self.is_dirty = true;
+    /// executors cache.
+    pub fn new_cached(executor: Arc<dyn Executor>) -> Self {
+        Self {
+            executor,
+            is_miss: false,
+            is_updated: false,
+        }
     }
-    pub fn get(&self, key: &Pubkey) -> Option<Arc<dyn Executor>> {
+
-        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<dyn Executor>) -> 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<dyn Executor>) -> 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<dyn Executor> {
+        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
+    /// Cache an executor that wasn't found in the cache
-    /// the work that can be re-used across executions
     pub fn add_executor(&self, pubkey: &Pubkey, executor: Arc<dyn Executor>) {
-        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<dyn Executor>) {
+        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<Arc<dyn Executor>> {
-        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

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,

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 {
+        Rc::new(RefCell::new(executors))
-            executors,
-            is_dirty: false,
-        }))
     }
 
     /// Add executors back to the bank's cache if modified
-    fn update_executors(&self, executors: Rc<RefCell<Executors>>) {
+    fn update_executors(&self, allow_updates: bool, executors: Rc<RefCell<Executors>>) {
         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() {
+            for (key, executor) in dirty_executors.into_iter() {
-                cache.put(key, (*executor).clone());
+                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<TransactionLogMessages> =
             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(key1, TransactionExecutor::new_cached(executor.clone()));
-        executors.insert(key2, executor.clone());
+        executors.insert(key2, TransactionExecutor::new_cached(executor.clone()));
-        executors.insert(key3, executor.clone());
+        executors.insert(key3, TransactionExecutor::new_cached(executor.clone()));
-        executors.insert(key4, executor.clone());
+        executors.insert(key4, TransactionExecutor::new_cached(executor.clone()));
         let executors = Rc::new(RefCell::new(executors));
-        executors.borrow_mut().is_dirty = false;
+        executors
-        bank.update_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(key1, TransactionExecutor::new_miss(executor.clone()));
-        executors.insert(key2, executor.clone());
+        executors.insert(key2, TransactionExecutor::new_miss(executor.clone()));
-        executors.insert(key3, executor.clone());
+        executors.insert(key3, TransactionExecutor::new_miss(executor.clone()));
-        executors.insert(key4, 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_eq!(executors.borrow().len(), 4);
-        assert!(executors.borrow().executors.contains_key(&key1));
+        assert!(executors.borrow().contains_key(&key1));
-        assert!(executors.borrow().executors.contains_key(&key2));
+        assert!(executors.borrow().contains_key(&key2));
-        assert!(executors.borrow().executors.contains_key(&key3));
+        assert!(executors.borrow().contains_key(&key3));
-        assert!(executors.borrow().executors.contains_key(&key4));
+        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_eq!(executors.borrow().len(), 4);
-        assert!(executors.borrow().executors.contains_key(&key1));
+        assert!(executors.borrow().contains_key(&key1));
-        assert!(executors.borrow().executors.contains_key(&key2));
+        assert!(executors.borrow().contains_key(&key2));
-        assert!(executors.borrow().executors.contains_key(&key3));
+        assert!(executors.borrow().contains_key(&key3));
-        assert!(executors.borrow().executors.contains_key(&key4));
+        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_eq!(executors.borrow().len(), 0);
-        assert!(!executors.borrow().executors.contains_key(&key1));
+        assert!(!executors.borrow().contains_key(&key1));
-        assert!(!executors.borrow().executors.contains_key(&key2));
+        assert!(!executors.borrow().contains_key(&key2));
-        assert!(!executors.borrow().executors.contains_key(&key3));
+        assert!(!executors.borrow().contains_key(&key3));
-        assert!(!executors.borrow().executors.contains_key(&key4));
+        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]