Separate stats updates from decision_maker (#30481)

* Separate stats updates from decision_maker

* BufferedPacketsDecision::bank_start

* BufferedPacketsDecision: bank_start() doc-comment

* remove unnecessary clone

core/src/banking_stage.rs

@@ -432,8 +432,9 @@ impl BankingStage {
         if unprocessed_transaction_storage.should_not_process() {
             return;
         }
-        let ((metrics_action, decision), make_decision_time) =
-            measure!(decision_maker.make_consume_or_forward_decision(slot_metrics_tracker));
+        let (decision, make_decision_time) =
+            measure!(decision_maker.make_consume_or_forward_decision());
+        let metrics_action = slot_metrics_tracker.check_leader_slot_boundary(decision.bank_start());
         slot_metrics_tracker.increment_make_decision_us(make_decision_time.as_us());
 
         match decision {

core/src/banking_stage/decision_maker.rs

@@ -1,5 +1,4 @@
 use {
-    crate::leader_slot_banking_stage_metrics::{LeaderSlotMetricsTracker, MetricsTrackerAction},
     solana_poh::poh_recorder::{BankStart, PohRecorder},
     solana_sdk::{
         clock::{
@@ -19,6 +18,16 @@ pub enum BufferedPacketsDecision {
     Hold,
 }
 
+impl BufferedPacketsDecision {
+    /// Returns the `BankStart` if the decision is `Consume`. Otherwise, returns `None`.
+    pub fn bank_start(&self) -> Option<&BankStart> {
+        match self {
+            Self::Consume(bank_start) => Some(bank_start),
+            _ => None,
+        }
+    }
+}
+
 pub struct DecisionMaker {
     my_pubkey: Pubkey,
     poh_recorder: Arc<RwLock<PohRecorder>>,
@@ -32,10 +41,7 @@ impl DecisionMaker {
         }
     }
 
-    pub(crate) fn make_consume_or_forward_decision(
-        &self,
-        slot_metrics_tracker: &mut LeaderSlotMetricsTracker,
-    ) -> (MetricsTrackerAction, BufferedPacketsDecision) {
+    pub(crate) fn make_consume_or_forward_decision(&self) -> BufferedPacketsDecision {
         let (leader_at_slot_offset, bank_start, would_be_leader, would_be_leader_shortly) = {
             let poh = self.poh_recorder.read().unwrap();
             let bank_start = poh
@@ -51,15 +57,12 @@ impl DecisionMaker {
             )
         };
 
-        (
-            slot_metrics_tracker.check_leader_slot_boundary(&bank_start),
-            Self::consume_or_forward_packets(
-                &self.my_pubkey,
-                leader_at_slot_offset,
-                bank_start,
-                would_be_leader,
-                would_be_leader_shortly,
-            ),
+        Self::consume_or_forward_packets(
+            &self.my_pubkey,
+            leader_at_slot_offset,
+            bank_start,
+            would_be_leader,
+            would_be_leader_shortly,
         )
     }
 
@@ -105,6 +108,23 @@ mod tests {
         std::{sync::Arc, time::Instant},
     };
 
+    #[test]
+    fn test_buffered_packet_decision_bank_start() {
+        let bank = Arc::new(Bank::default_for_tests());
+        let bank_start = BankStart {
+            working_bank: bank,
+            bank_creation_time: Arc::new(Instant::now()),
+        };
+        assert!(BufferedPacketsDecision::Consume(bank_start)
+            .bank_start()
+            .is_some());
+        assert!(BufferedPacketsDecision::Forward.bank_start().is_none());
+        assert!(BufferedPacketsDecision::ForwardAndHold
+            .bank_start()
+            .is_none());
+        assert!(BufferedPacketsDecision::Hold.bank_start().is_none());
+    }
+
     #[test]
     fn test_should_process_or_forward_packets() {
         let my_pubkey = solana_sdk::pubkey::new_rand();

core/src/leader_slot_banking_stage_metrics.rs

@@ -371,7 +371,7 @@ impl LeaderSlotMetricsTracker {
     // Check leader slot, return MetricsTrackerAction to be applied by apply_action()
     pub(crate) fn check_leader_slot_boundary(
         &mut self,
-        bank_start: &Option<BankStart>,
+        bank_start: Option<&BankStart>,
     ) -> MetricsTrackerAction {
         match (self.leader_slot_metrics.as_mut(), bank_start) {
             (None, None) => MetricsTrackerAction::Noop,
@@ -896,7 +896,7 @@ mod tests {
             ..
         } = setup_test_slot_boundary_banks();
         // Test that with no bank being tracked, and no new bank being tracked, nothing is reported
-        let action = leader_slot_metrics_tracker.check_leader_slot_boundary(&None);
+        let action = leader_slot_metrics_tracker.check_leader_slot_boundary(None);
         assert_eq!(
             mem::discriminant(&MetricsTrackerAction::Noop),
             mem::discriminant(&action)
@@ -917,7 +917,7 @@ mod tests {
         // Metrics should not be reported because leader slot has not ended
         assert!(leader_slot_metrics_tracker.leader_slot_metrics.is_none());
         let action =
-            leader_slot_metrics_tracker.check_leader_slot_boundary(&Some(first_poh_recorder_bank));
+            leader_slot_metrics_tracker.check_leader_slot_boundary(Some(&first_poh_recorder_bank));
         assert_eq!(
             mem::discriminant(&MetricsTrackerAction::NewTracker(None)),
             mem::discriminant(&action)
@@ -941,12 +941,12 @@ mod tests {
         {
             // Setup first_bank
             let action = leader_slot_metrics_tracker
-                .check_leader_slot_boundary(&Some(first_poh_recorder_bank));
+                .check_leader_slot_boundary(Some(&first_poh_recorder_bank));
             assert!(leader_slot_metrics_tracker.apply_action(action).is_none());
         }
         {
             // Assert reporting if slot has ended
-            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(&None);
+            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(None);
             assert_eq!(
                 mem::discriminant(&MetricsTrackerAction::ReportAndResetTracker),
                 mem::discriminant(&action)
@@ -959,7 +959,7 @@ mod tests {
         }
         {
             // Assert no-op if still no new bank
-            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(&None);
+            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(None);
             assert_eq!(
                 mem::discriminant(&MetricsTrackerAction::Noop),
                 mem::discriminant(&action)
@@ -981,13 +981,13 @@ mod tests {
         {
             // Setup with first_bank
             let action = leader_slot_metrics_tracker
-                .check_leader_slot_boundary(&Some(first_poh_recorder_bank.clone()));
+                .check_leader_slot_boundary(Some(&first_poh_recorder_bank));
             assert!(leader_slot_metrics_tracker.apply_action(action).is_none());
         }
         {
             // Assert nop-op if same bank
             let action = leader_slot_metrics_tracker
-                .check_leader_slot_boundary(&Some(first_poh_recorder_bank));
+                .check_leader_slot_boundary(Some(&first_poh_recorder_bank));
             assert_eq!(
                 mem::discriminant(&MetricsTrackerAction::Noop),
                 mem::discriminant(&action)
@@ -996,7 +996,7 @@ mod tests {
         }
         {
             // Assert reporting if slot has ended
-            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(&None);
+            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(None);
             assert_eq!(
                 mem::discriminant(&MetricsTrackerAction::ReportAndResetTracker),
                 mem::discriminant(&action)
@@ -1025,13 +1025,13 @@ mod tests {
         {
             // Setup with first_bank
             let action = leader_slot_metrics_tracker
-                .check_leader_slot_boundary(&Some(first_poh_recorder_bank));
+                .check_leader_slot_boundary(Some(&first_poh_recorder_bank));
             assert!(leader_slot_metrics_tracker.apply_action(action).is_none());
         }
         {
             // Assert reporting if new bank
             let action = leader_slot_metrics_tracker
-                .check_leader_slot_boundary(&Some(next_poh_recorder_bank));
+                .check_leader_slot_boundary(Some(&next_poh_recorder_bank));
             assert_eq!(
                 mem::discriminant(&MetricsTrackerAction::ReportAndNewTracker(None)),
                 mem::discriminant(&action)
@@ -1044,7 +1044,7 @@ mod tests {
         }
         {
             // Assert reporting if slot has ended
-            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(&None);
+            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(None);
             assert_eq!(
                 mem::discriminant(&MetricsTrackerAction::ReportAndResetTracker),
                 mem::discriminant(&action)
@@ -1072,13 +1072,13 @@ mod tests {
         {
             // Setup with next_bank
             let action = leader_slot_metrics_tracker
-                .check_leader_slot_boundary(&Some(next_poh_recorder_bank));
+                .check_leader_slot_boundary(Some(&next_poh_recorder_bank));
             assert!(leader_slot_metrics_tracker.apply_action(action).is_none());
         }
         {
             // Assert reporting if new bank
             let action = leader_slot_metrics_tracker
-                .check_leader_slot_boundary(&Some(first_poh_recorder_bank));
+                .check_leader_slot_boundary(Some(&first_poh_recorder_bank));
             assert_eq!(
                 mem::discriminant(&MetricsTrackerAction::ReportAndNewTracker(None)),
                 mem::discriminant(&action)
@@ -1091,7 +1091,7 @@ mod tests {
         }
         {
             // Assert reporting if slot has ended
-            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(&None);
+            let action = leader_slot_metrics_tracker.check_leader_slot_boundary(None);
             assert_eq!(
                 mem::discriminant(&MetricsTrackerAction::ReportAndResetTracker),
                 mem::discriminant(&action)