From c847236147f5b2e7db57ef4e65fe0f0766091567 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Wed, 12 Apr 2023 21:40:59 -0700 Subject: [PATCH] decision maker perf (#30618) --- core/src/banking_stage/decision_maker.rs | 145 +++++++++++------------ 1 file changed, 66 insertions(+), 79 deletions(-) diff --git a/core/src/banking_stage/decision_maker.rs b/core/src/banking_stage/decision_maker.rs index 2077095e89..3e171923c4 100644 --- a/core/src/banking_stage/decision_maker.rs +++ b/core/src/banking_stage/decision_maker.rs @@ -42,50 +42,51 @@ impl DecisionMaker { } 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 - .bank_start() - .filter(|bank_start| bank_start.should_working_bank_still_be_processing_txs()); - ( - poh.leader_after_n_slots(FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET), - bank_start, - poh.would_be_leader(HOLD_TRANSACTIONS_SLOT_OFFSET * DEFAULT_TICKS_PER_SLOT), - poh.would_be_leader( - (FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET - 1) * DEFAULT_TICKS_PER_SLOT, - ), - ) - }; + let decision; + { + let poh_recorder = self.poh_recorder.read().unwrap(); + decision = Self::consume_or_forward_packets( + &self.my_pubkey, + || { + poh_recorder.bank_start().filter(|bank_start| { + bank_start.should_working_bank_still_be_processing_txs() + }) + }, + || { + poh_recorder + .would_be_leader(HOLD_TRANSACTIONS_SLOT_OFFSET * DEFAULT_TICKS_PER_SLOT) + }, + || { + poh_recorder + .would_be_leader(HOLD_TRANSACTIONS_SLOT_OFFSET * DEFAULT_TICKS_PER_SLOT) + }, + || poh_recorder.leader_after_n_slots(FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET), + ); + } - Self::consume_or_forward_packets( - &self.my_pubkey, - leader_at_slot_offset, - bank_start, - would_be_leader, - would_be_leader_shortly, - ) + decision } fn consume_or_forward_packets( my_pubkey: &Pubkey, - leader_pubkey: Option, - bank_start: Option, - would_be_leader: bool, - would_be_leader_shortly: bool, + bank_start_fn: impl FnOnce() -> Option, + would_be_leader_shortly_fn: impl FnOnce() -> bool, + would_be_leader_fn: impl FnOnce() -> bool, + leader_pubkey_fn: impl FnOnce() -> Option, ) -> BufferedPacketsDecision { // If has active bank, then immediately process buffered packets // otherwise, based on leader schedule to either forward or hold packets - if let Some(bank_start) = bank_start { + if let Some(bank_start) = bank_start_fn() { // If the bank is available, this node is the leader BufferedPacketsDecision::Consume(bank_start) - } else if would_be_leader_shortly { + } else if would_be_leader_shortly_fn() { // If the node will be the leader soon, hold the packets for now BufferedPacketsDecision::Hold - } else if would_be_leader { + } else if would_be_leader_fn() { // Node will be leader within ~20 slots, hold the transactions in // case it is the only node which produces an accepted slot. BufferedPacketsDecision::ForwardAndHold - } else if let Some(x) = leader_pubkey { + } else if let Some(x) = leader_pubkey_fn() { if x != *my_pubkey { // If the current node is not the leader, forward the buffered packets BufferedPacketsDecision::Forward @@ -104,6 +105,7 @@ impl DecisionMaker { mod tests { use { super::*, + core::panic, solana_runtime::bank::Bank, std::{sync::Arc, time::Instant}, }; @@ -138,82 +140,67 @@ mod tests { assert_matches!( DecisionMaker::consume_or_forward_packets( &my_pubkey, - None, - bank_start.clone(), - false, - false + || bank_start.clone(), + || panic!("should not be called"), + || panic!("should not be called"), + || panic!("should not be called") ), BufferedPacketsDecision::Consume(_) ); - assert_matches!( - DecisionMaker::consume_or_forward_packets(&my_pubkey, None, None, false, false), - BufferedPacketsDecision::Hold - ); - assert_matches!( - DecisionMaker::consume_or_forward_packets(&my_pubkey1, None, None, false, false), - BufferedPacketsDecision::Hold - ); - + // Unknown leader, hold the packets assert_matches!( DecisionMaker::consume_or_forward_packets( &my_pubkey, - Some(my_pubkey1), - None, - false, - false + || None, + || false, + || false, + || None + ), + BufferedPacketsDecision::Hold + ); + // Leader other than me, forward the packets + assert_matches!( + DecisionMaker::consume_or_forward_packets( + &my_pubkey, + || None, + || false, + || false, + || Some(my_pubkey1), ), BufferedPacketsDecision::Forward ); - + // Will be leader shortly, hold the packets assert_matches!( DecisionMaker::consume_or_forward_packets( &my_pubkey, - Some(my_pubkey1), - None, - true, - true + || None, + || true, + || panic!("should not be called"), + || panic!("should not be called"), ), BufferedPacketsDecision::Hold ); + // Will be leader (not shortly), forward and hold assert_matches!( DecisionMaker::consume_or_forward_packets( &my_pubkey, - Some(my_pubkey1), - None, - true, - false + || None, + || false, + || true, + || panic!("should not be called"), ), BufferedPacketsDecision::ForwardAndHold ); - assert_matches!( - DecisionMaker::consume_or_forward_packets( - &my_pubkey, - Some(my_pubkey1), - bank_start.clone(), - false, - false - ), - BufferedPacketsDecision::Consume(_) - ); + // Current leader matches my pubkey, hold assert_matches!( DecisionMaker::consume_or_forward_packets( &my_pubkey1, - Some(my_pubkey1), - None, - false, - false + || None, + || false, + || false, + || Some(my_pubkey1), ), BufferedPacketsDecision::Hold ); - assert_matches!( - DecisionMaker::consume_or_forward_packets( - &my_pubkey1, - Some(my_pubkey1), - bank_start, - false, - false - ), - BufferedPacketsDecision::Consume(_) - ); } }