cost model nits (#18528)

core/src/cost_model.rs

@@ -92,7 +92,7 @@ impl CostModel {
 
     pub fn initialize_cost_table(&mut self, cost_table: &[(Pubkey, u64)]) {
         for (program_id, cost) in cost_table {
-            match self.upsert_instruction_cost(program_id, cost) {
+            match self.upsert_instruction_cost(program_id, *cost) {
                 Ok(c) => {
                     debug!(
                         "initiating cost table, instruction {:?} has cost {}",
@@ -147,7 +147,7 @@ impl CostModel {
     pub fn upsert_instruction_cost(
         &mut self,
         program_key: &Pubkey,
-        cost: &u64,
+        cost: u64,
     ) -> Result<u64, &'static str> {
         self.instruction_execution_cost_table
             .upsert(program_key, cost);
@@ -232,12 +232,12 @@ mod tests {
         let mut testee = CostModel::default();
 
         let known_key = Pubkey::from_str("known11111111111111111111111111111111111111").unwrap();
-        testee.upsert_instruction_cost(&known_key, &100).unwrap();
+        testee.upsert_instruction_cost(&known_key, 100).unwrap();
         // find cost for known programs
         assert_eq!(100, testee.find_instruction_cost(&known_key));
 
         testee
-            .upsert_instruction_cost(&bpf_loader::id(), &1999)
+            .upsert_instruction_cost(&bpf_loader::id(), 1999)
             .unwrap();
         assert_eq!(1999, testee.find_instruction_cost(&bpf_loader::id()));
 
@@ -267,7 +267,7 @@ mod tests {
 
         let mut testee = CostModel::default();
         testee
-            .upsert_instruction_cost(&system_program::id(), &expected_cost)
+            .upsert_instruction_cost(&system_program::id(), expected_cost)
             .unwrap();
         assert_eq!(
             expected_cost,
@@ -293,7 +293,7 @@ mod tests {
 
         let mut testee = CostModel::default();
         testee
-            .upsert_instruction_cost(&system_program::id(), &program_cost)
+            .upsert_instruction_cost(&system_program::id(), program_cost)
             .unwrap();
         assert_eq!(expected_cost, testee.find_transaction_cost(&tx));
     }
@@ -371,7 +371,7 @@ mod tests {
         );
 
         // insert instruction cost to table
-        assert!(cost_model.upsert_instruction_cost(&key1, &cost1).is_ok());
+        assert!(cost_model.upsert_instruction_cost(&key1, cost1).is_ok());
 
         // now it is known insturction with known cost
         assert_eq!(cost1, cost_model.find_instruction_cost(&key1));
@@ -390,7 +390,7 @@ mod tests {
 
         let mut cost_model = CostModel::default();
         cost_model
-            .upsert_instruction_cost(&system_program::id(), &expected_execution_cost)
+            .upsert_instruction_cost(&system_program::id(), expected_execution_cost)
             .unwrap();
         let tx_cost = cost_model.calculate_cost(&tx);
         assert_eq!(expected_account_cost, tx_cost.account_access_cost);
@@ -408,11 +408,11 @@ mod tests {
         let mut cost_model = CostModel::default();
 
         // insert instruction cost to table
-        assert!(cost_model.upsert_instruction_cost(&key1, &cost1).is_ok());
+        assert!(cost_model.upsert_instruction_cost(&key1, cost1).is_ok());
         assert_eq!(cost1, cost_model.find_instruction_cost(&key1));
 
         // update instruction cost
-        assert!(cost_model.upsert_instruction_cost(&key1, &cost2).is_ok());
+        assert!(cost_model.upsert_instruction_cost(&key1, cost2).is_ok());
         assert_eq!(updated_cost, cost_model.find_instruction_cost(&key1));
     }
 
@@ -454,8 +454,8 @@ mod tests {
                 if i == 5 {
                     thread::spawn(move || {
                         let mut cost_model = cost_model.write().unwrap();
-                        assert!(cost_model.upsert_instruction_cost(&prog1, &cost1).is_ok());
-                        assert!(cost_model.upsert_instruction_cost(&prog2, &cost2).is_ok());
+                        assert!(cost_model.upsert_instruction_cost(&prog1, cost1).is_ok());
+                        assert!(cost_model.upsert_instruction_cost(&prog2, cost2).is_ok());
                     })
                 } else {
                     thread::spawn(move || {

core/src/cost_tracker.rs

@@ -1,4 +1,4 @@
-//! `cost_tracker` keeps tracking tranasction cost per chained accounts as well as for entire block
+//! `cost_tracker` keeps tracking transaction cost per chained accounts as well as for entire block
 //! It aggregates `cost_model`, which provides service of calculating transaction cost.
 //! The main functions are:
 //! - would_transaction_fit(&tx), immutable function to test if `tx` would fit into current block

core/src/cost_update_service.rs

@@ -135,26 +135,27 @@ impl CostUpdateService {
 
     fn update_cost_model(cost_model: &RwLock<CostModel>, execute_timings: &ExecuteTimings) -> bool {
         let mut dirty = false;
-        let mut cost_model_mutable = cost_model.write().unwrap();
-        for (program_id, stats) in &execute_timings.details.per_program_timings {
-            let cost = stats.0 / stats.1 as u64;
-            match cost_model_mutable.upsert_instruction_cost(program_id, &cost) {
-                Ok(c) => {
-                    debug!(
-                        "after replayed into bank, instruction {:?} has averaged cost {}",
-                        program_id, c
-                    );
-                    dirty = true;
-                }
-                Err(err) => {
-                    debug!(
+        {
+            let mut cost_model_mutable = cost_model.write().unwrap();
+            for (program_id, tining) in &execute_timings.details.per_program_timings {
+                let cost = tining.accumulated_us / tining.count as u64;
+                match cost_model_mutable.upsert_instruction_cost(program_id, cost) {
+                    Ok(c) => {
+                        debug!(
+                            "after replayed into bank, instruction {:?} has averaged cost {}",
+                            program_id, c
+                        );
+                        dirty = true;
+                    }
+                    Err(err) => {
+                        debug!(
                         "after replayed into bank, instruction {:?} failed to update cost, err: {}",
                         program_id, err
                     );
+                    }
                 }
             }
         }
-        drop(cost_model_mutable);
         debug!(
            "after replayed into bank, updated cost model instruction cost table, current values: {:?}",
            cost_model.read().unwrap().get_instruction_cost_table()
@@ -187,6 +188,7 @@ impl CostUpdateService {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use solana_runtime::message_processor::ProgramTiming;
     use solana_sdk::pubkey::Pubkey;
 
     #[test]
@@ -219,10 +221,13 @@ mod tests {
             let count: u32 = 10;
             expected_cost = accumulated_us / count as u64;
 
-            execute_timings
-                .details
-                .per_program_timings
-                .insert(program_key_1, (accumulated_us, count));
+            execute_timings.details.per_program_timings.insert(
+                program_key_1,
+                ProgramTiming {
+                    accumulated_us,
+                    count,
+                },
+            );
             CostUpdateService::update_cost_model(&cost_model, &execute_timings);
             assert_eq!(
                 1,
@@ -249,10 +254,13 @@ mod tests {
             // to expect new cost is Average(new_value, existing_value)
             expected_cost = ((accumulated_us / count as u64) + expected_cost) / 2;
 
-            execute_timings
-                .details
-                .per_program_timings
-                .insert(program_key_1, (accumulated_us, count));
+            execute_timings.details.per_program_timings.insert(
+                program_key_1,
+                ProgramTiming {
+                    accumulated_us,
+                    count,
+                },
+            );
             CostUpdateService::update_cost_model(&cost_model, &execute_timings);
             assert_eq!(
                 1,

core/src/execute_cost_table.rs

@@ -78,15 +78,15 @@ impl ExecuteCostTable {
         self.table.get(key)
     }
 
-    pub fn upsert(&mut self, key: &Pubkey, value: &u64) {
+    pub fn upsert(&mut self, key: &Pubkey, value: u64) {
         let need_to_add = self.table.get(key).is_none();
         let current_size = self.get_count();
         if current_size == self.capacity && need_to_add {
             self.prune_to(&((current_size as f64 * PRUNE_RATIO) as usize));
         }
 
-        let program_cost = self.table.entry(*key).or_insert(*value);
-        *program_cost = (*program_cost + *value) / 2;
+        let program_cost = self.table.entry(*key).or_insert(value);
+        *program_cost = (*program_cost + value) / 2;
 
         let (count, timestamp) = self
             .occurrences
@@ -154,9 +154,9 @@ mod tests {
         let key2 = Pubkey::new_unique();
         let key3 = Pubkey::new_unique();
 
-        testee.upsert(&key1, &1);
-        testee.upsert(&key2, &2);
-        testee.upsert(&key3, &3);
+        testee.upsert(&key1, 1);
+        testee.upsert(&key2, 2);
+        testee.upsert(&key3, 3);
 
         testee.prune_to(&(capacity - 1));
 
@@ -176,10 +176,10 @@ mod tests {
         let key2 = Pubkey::new_unique();
         let key3 = Pubkey::new_unique();
 
-        testee.upsert(&key1, &1);
-        testee.upsert(&key1, &1);
-        testee.upsert(&key2, &2);
-        testee.upsert(&key3, &3);
+        testee.upsert(&key1, 1);
+        testee.upsert(&key1, 1);
+        testee.upsert(&key2, 2);
+        testee.upsert(&key3, 3);
 
         testee.prune_to(&(capacity - 1));
 
@@ -204,22 +204,22 @@ mod tests {
         assert!(testee.get_cost(&key1).is_none());
 
         // insert one record
-        testee.upsert(&key1, &cost1);
+        testee.upsert(&key1, cost1);
         assert_eq!(1, testee.get_count());
         assert_eq!(cost1, testee.get_average());
         assert_eq!(cost1, testee.get_mode());
         assert_eq!(&cost1, testee.get_cost(&key1).unwrap());
 
         // insert 2nd record
-        testee.upsert(&key2, &cost2);
+        testee.upsert(&key2, cost2);
         assert_eq!(2, testee.get_count());
         assert_eq!((cost1 + cost2) / 2_u64, testee.get_average());
         assert_eq!(cost2, testee.get_mode());
         assert_eq!(&cost1, testee.get_cost(&key1).unwrap());
         assert_eq!(&cost2, testee.get_cost(&key2).unwrap());
 
         // update 1st record
-        testee.upsert(&key1, &cost2);
+        testee.upsert(&key1, cost2);
         assert_eq!(2, testee.get_count());
         assert_eq!(((cost1 + cost2) / 2 + cost2) / 2, testee.get_average());
         assert_eq!((cost1 + cost2) / 2, testee.get_mode());
@@ -243,18 +243,18 @@ mod tests {
         let cost4: u64 = 130;
 
         // insert one record
-        testee.upsert(&key1, &cost1);
+        testee.upsert(&key1, cost1);
         assert_eq!(1, testee.get_count());
         assert_eq!(&cost1, testee.get_cost(&key1).unwrap());
 
         // insert 2nd record
-        testee.upsert(&key2, &cost2);
+        testee.upsert(&key2, cost2);
         assert_eq!(2, testee.get_count());
         assert_eq!(&cost1, testee.get_cost(&key1).unwrap());
         assert_eq!(&cost2, testee.get_cost(&key2).unwrap());
 
         // insert 3rd record, pushes out the oldest (eg 1st) record
-        testee.upsert(&key3, &cost3);
+        testee.upsert(&key3, cost3);
         assert_eq!(2, testee.get_count());
         assert_eq!((cost2 + cost3) / 2_u64, testee.get_average());
         assert_eq!(cost3, testee.get_mode());
@@ -264,8 +264,8 @@ mod tests {
 
         // update 2nd record, so the 3rd becomes the oldest
         // add 4th record, pushes out 3rd key
-        testee.upsert(&key2, &cost1);
-        testee.upsert(&key4, &cost4);
+        testee.upsert(&key2, cost1);
+        testee.upsert(&key4, cost4);
         assert_eq!(((cost1 + cost2) / 2 + cost4) / 2_u64, testee.get_average());
         assert_eq!((cost1 + cost2) / 2, testee.get_mode());
         assert_eq!(2, testee.get_count());

core/src/progress_map.rs

@@ -121,13 +121,13 @@ impl ReplaySlotStats {
             .per_program_timings
             .iter()
             .collect();
-        per_pubkey_timings.sort_by(|a, b| b.1 .0.cmp(&a.1 .0));
-        let total: u64 = per_pubkey_timings.iter().map(|a| a.1 .0).sum();
+        per_pubkey_timings.sort_by(|a, b| b.1.accumulated_us.cmp(&a.1.accumulated_us));
+        let total: u64 = per_pubkey_timings.iter().map(|a| a.1.accumulated_us).sum();
         for (pubkey, time) in per_pubkey_timings.iter().take(5) {
             datapoint_info!(
                 "per_program_timings",
                 ("pubkey", pubkey.to_string(), String),
-                ("execute_us", time.0, i64)
+                ("execute_us", time.accumulated_us, i64)
             );
         }
         datapoint_info!(

runtime/src/message_processor.rs

@@ -56,6 +56,12 @@ impl Executors {
     }
 }
 
+#[derive(Default, Debug)]
+pub struct ProgramTiming {
+    pub accumulated_us: u64,
+    pub count: u32,
+}
+
 #[derive(Default, Debug)]
 pub struct ExecuteDetailsTimings {
     pub serialize_us: u64,
@@ -66,7 +72,7 @@ pub struct ExecuteDetailsTimings {
     pub total_account_count: u64,
     pub total_data_size: usize,
     pub data_size_changed: usize,
-    pub per_program_timings: HashMap<Pubkey, (u64, u32)>,
+    pub per_program_timings: HashMap<Pubkey, ProgramTiming>,
 }
 
 impl ExecuteDetailsTimings {
@@ -81,8 +87,8 @@ impl ExecuteDetailsTimings {
         self.data_size_changed += other.data_size_changed;
         for (id, other) in &other.per_program_timings {
             let time_count = self.per_program_timings.entry(*id).or_default();
-            time_count.0 += other.0;
-            time_count.1 += other.1;
+            time_count.accumulated_us += other.accumulated_us;
+            time_count.count += other.count;
         }
     }
 }
@@ -1244,8 +1250,8 @@ impl MessageProcessor {
 
             let program_id = instruction.program_id(&message.account_keys);
             let time_count = timings.per_program_timings.entry(*program_id).or_default();
-            time_count.0 += time.as_us();
-            time_count.1 += 1;
+            time_count.accumulated_us += time.as_us();
+            time_count.count += 1;
 
             err?;
         }