From b23420bdf38c504c3849f39c192fa3fd6626d2ad Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 12 Oct 2021 12:11:51 -0500 Subject: [PATCH] wip - remove cost_tracker from tpu; cost_model return tx_cost to clean up calculate_cost read op api; --- banking-bench/src/main.rs | 6 +-- core/benches/banking_stage.rs | 9 ++-- core/src/banking_stage.rs | 54 ++++++++++++------------ ledger-tool/src/main.rs | 6 +-- runtime/src/cost_model.rs | 43 +++++++++---------- runtime/src/cost_tracker.rs | 79 +++++++++-------------------------- 6 files changed, 76 insertions(+), 121 deletions(-) diff --git a/banking-bench/src/main.rs b/banking-bench/src/main.rs index baec1c05f31a86..27e8dde3261bff 100644 --- a/banking-bench/src/main.rs +++ b/banking-bench/src/main.rs @@ -16,7 +16,7 @@ use solana_perf::packet::to_packets_chunked; use solana_poh::poh_recorder::{create_test_recorder, PohRecorder, WorkingBankEntry}; use solana_runtime::{ accounts_background_service::AbsRequestSender, bank::Bank, bank_forks::BankForks, - cost_model::CostModel, cost_tracker::CostTracker, + cost_model::CostModel, }; use solana_sdk::{ hash::Hash, @@ -233,9 +233,7 @@ fn main() { vote_receiver, None, replay_vote_sender, - Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( - CostModel::default(), - ))))), + Arc::new(RwLock::new(CostModel::default())), ); poh_recorder.lock().unwrap().set_bank(&bank); diff --git a/core/benches/banking_stage.rs b/core/benches/banking_stage.rs index 479966538e8862..63b5c061140b40 100644 --- a/core/benches/banking_stage.rs +++ b/core/benches/banking_stage.rs @@ -19,7 +19,6 @@ use solana_perf::test_tx::test_tx; use solana_poh::poh_recorder::{create_test_recorder, WorkingBankEntry}; use solana_runtime::bank::Bank; use solana_runtime::cost_model::CostModel; -use solana_runtime::cost_tracker::CostTracker; use solana_runtime::cost_tracker_stats::CostTrackerStats; use solana_sdk::genesis_config::GenesisConfig; use solana_sdk::hash::Hash; @@ -95,9 +94,9 @@ fn bench_consume_buffered(bencher: &mut Bencher) { None::>, &BankingStageStats::default(), &recorder, - &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( + &Arc::new(RwLock::new( CostModel::new(std::u64::MAX, std::u64::MAX), - ))))), + )), &mut CostTrackerStats::default(), ); }); @@ -225,9 +224,9 @@ fn bench_banking(bencher: &mut Bencher, tx_type: TransactionType) { vote_receiver, None, s, - Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( + Arc::new(RwLock::new( CostModel::new(std::u64::MAX, std::u64::MAX), - ))))), + )), ); poh_recorder.lock().unwrap().set_bank(&bank); diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 4f9f1c47d83616..53b9e8f1f04f0c 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -20,12 +20,12 @@ use solana_perf::{ use solana_poh::poh_recorder::{BankStart, PohRecorder, PohRecorderError, TransactionRecorder}; use solana_runtime::{ accounts_db::ErrorCounters, - cost_model::CostModel, bank::{ Bank, ExecuteTimings, TransactionBalancesSet, TransactionCheckResult, TransactionExecutionResult, }, bank_utils, + cost_model::CostModel, cost_tracker::CostTracker, cost_tracker_stats::CostTrackerStats, transaction_batch::TransactionBatch, @@ -963,10 +963,9 @@ impl BankingStage { transaction_status_sender: Option, gossip_vote_sender: &ReplayVoteSender, ) -> (Result, Vec) { - - // TODO TAO - before locking account for txs, do - // 1. calculate TXs costs: cost_model.calculate_costs(txs) -> [transaction_cost] - // 2, combine txs and [transaction_costs] into txs_costs, do packing logic: + // TODO TAO - before locking account for txs, do + // 1. calculate TXs costs: cost_model.calculate_costs(txs) -> [transaction_cost] + // 2, combine txs and [transaction_costs] into txs_costs, do packing logic: // for (tx, cost) { bank.cost_tracker_mutable().add(tx, cost)->result; // for OK: tx into candidates_txs list; // for err: tx into retryable_txs; @@ -1155,7 +1154,7 @@ impl BankingStage { &tx, // TODO TAO - refactor transaction_cost to avoid vec, use index[] // instead; so it can be readGuard - cost_model + &cost_model .read() .unwrap() .calculate_cost(&tx, demote_program_write_locks), @@ -1288,18 +1287,15 @@ impl BankingStage { if unprocessed_tx_indexes.iter().all(|&i| i != index) { bank.write_cost_tracker().unwrap().add_transaction_cost( tx, - // TODO TAO - refactor transaction_cost to avoid vec, use index[] - // instead; so it can be readGuard - // TODO TAO - refactor to avoid second cost_model calculation. Should cache - // first calc results and re-use, also need to resue later for bankless when to - // convert to fee to check account balance. - cost_model - .read() - .unwrap() - .calculate_cost( - tx, - bank.demote_program_write_locks() - ), + // TODO TAO - refactor transaction_cost to avoid vec, use index[] + // instead; so it can be readGuard + // TODO TAO - refactor to avoid second cost_model calculation. Should cache + // first calc results and re-use, also need to resue later for bankless when to + // convert to fee to check account balance. + &cost_model + .read() + .unwrap() + .calculate_cost(tx, bank.demote_program_write_locks()), cost_tracker_stats, ); } @@ -1756,7 +1752,7 @@ mod tests { gossip_verified_vote_receiver, None, gossip_vote_sender, - &Arc::new(RwLock::new(CostModel::default())), + Arc::new(RwLock::new(CostModel::default())), ); drop(verified_sender); drop(gossip_verified_vote_sender); @@ -1805,7 +1801,7 @@ mod tests { verified_gossip_vote_receiver, None, gossip_vote_sender, - &Arc::new(RwLock::new(CostModel::default())), + Arc::new(RwLock::new(CostModel::default())), ); trace!("sending bank"); drop(verified_sender); @@ -1878,7 +1874,7 @@ mod tests { gossip_verified_vote_receiver, None, gossip_vote_sender, - &Arc::new(RwLock::new(CostModel::default())), + Arc::new(RwLock::new(CostModel::default())), ); // fund another account so we can send 2 good transactions in a single batch. @@ -2029,7 +2025,7 @@ mod tests { 3, None, gossip_vote_sender, - &Arc::new(RwLock::new(CostModel::default())), + Arc::new(RwLock::new(CostModel::default())), ); // wait for banking_stage to eat the packets @@ -2828,9 +2824,9 @@ mod tests { None::>, &BankingStageStats::default(), &recorder, - &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( + &Arc::new(RwLock::new( CostModel::default(), - ))))), + )), &mut CostTrackerStats::default(), ); assert_eq!(buffered_packets[0].1.len(), num_conflicting_transactions); @@ -2915,9 +2911,9 @@ mod tests { test_fn, &BankingStageStats::default(), &recorder, - &Arc::new(RwLock::new(CostTracker::new(Arc::new(RwLock::new( + &Arc::new(RwLock::new( CostModel::default(), - ))))), + )), &mut CostTrackerStats::default(), ); @@ -3177,6 +3173,7 @@ mod tests { &BankingStageStats::default(), false, votes_only, + &Arc::new(RwLock::new(CostModel::default())), &mut CostTrackerStats::default(), ); assert_eq!(2, txs.len()); @@ -3192,6 +3189,7 @@ mod tests { &BankingStageStats::default(), false, votes_only, + &Arc::new(RwLock::new(CostModel::default())), &mut CostTrackerStats::default(), ); assert_eq!(0, txs.len()); @@ -3216,6 +3214,7 @@ mod tests { &BankingStageStats::default(), false, votes_only, + &Arc::new(RwLock::new(CostModel::default())), &mut CostTrackerStats::default(), ); assert_eq!(3, txs.len()); @@ -3231,6 +3230,7 @@ mod tests { &BankingStageStats::default(), false, votes_only, + &Arc::new(RwLock::new(CostModel::default())), &mut CostTrackerStats::default(), ); assert_eq!(2, txs.len()); @@ -3255,6 +3255,7 @@ mod tests { &BankingStageStats::default(), false, votes_only, + &Arc::new(RwLock::new(CostModel::default())), &mut CostTrackerStats::default(), ); assert_eq!(3, txs.len()); @@ -3270,6 +3271,7 @@ mod tests { &BankingStageStats::default(), false, votes_only, + &Arc::new(RwLock::new(CostModel::default())), &mut CostTrackerStats::default(), ); assert_eq!(3, txs.len()); diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 69966eae80a988..42bf07df39df67 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -774,13 +774,11 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String> let mut program_ids = HashMap::new(); let mut cost_model = CostModel::default(); cost_model.initialize_cost_table(&blockstore.read_program_costs().unwrap()); - let cost_model = Arc::new(RwLock::new(cost_model)); - let mut cost_tracker = CostTracker::new(cost_model.clone()); + let mut cost_tracker = CostTracker::default(); let mut cost_tracker_stats = CostTrackerStats::default(); for entry in entries { num_transactions += entry.transactions.len(); - let mut cost_model = cost_model.write().unwrap(); entry .transactions .into_iter() @@ -801,7 +799,7 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String> true, // demote_program_write_locks ); if cost_tracker - .try_add(tx_cost, &mut cost_tracker_stats) + .try_add(&transaction, &tx_cost, &mut cost_tracker_stats) .is_err() { println!( diff --git a/runtime/src/cost_model.rs b/runtime/src/cost_model.rs index ad801aca0c07e8..8c7b534307d108 100644 --- a/runtime/src/cost_model.rs +++ b/runtime/src/cost_model.rs @@ -41,14 +41,12 @@ impl TransactionCost { } } +// TODO TAO - does CostMOdel needs AbiExample? #[derive(AbiExample, Debug)] pub struct CostModel { account_cost_limit: u64, block_cost_limit: u64, instruction_execution_cost_table: ExecuteCostTable, - - // reusable variables - transaction_cost: TransactionCost, } impl Default for CostModel { @@ -58,12 +56,11 @@ impl Default for CostModel { } impl CostModel { - pub fn new(chain_max: u64, block_max: u64) -> Self { + pub fn new(account_max: u64, block_max: u64) -> Self { Self { - account_cost_limit: chain_max, + account_cost_limit: account_max, block_cost_limit: block_max, instruction_execution_cost_table: ExecuteCostTable::default(), - transaction_cost: TransactionCost::new_with_capacity(MAX_WRITABLE_ACCOUNTS), } } @@ -106,22 +103,19 @@ impl CostModel { } pub fn calculate_cost( - &mut self, + &self, transaction: &SanitizedTransaction, demote_program_write_locks: bool, - ) -> &TransactionCost { - self.transaction_cost.reset(); + ) -> TransactionCost { + let mut tx_cost = TransactionCost::new_with_capacity(MAX_WRITABLE_ACCOUNTS); - self.transaction_cost.signature_cost = self.get_signature_cost(transaction); - self.get_write_lock_cost(transaction, demote_program_write_locks); - self.transaction_cost.data_bytes_cost = self.get_data_bytes_cost(transaction); - self.transaction_cost.execution_cost = self.get_transaction_cost(transaction); + tx_cost.signature_cost = self.get_signature_cost(transaction); + self.get_write_lock_cost(&mut tx_cost, transaction, demote_program_write_locks); + tx_cost.data_bytes_cost = self.get_data_bytes_cost(transaction); + tx_cost.execution_cost = self.get_transaction_cost(transaction); - debug!( - "transaction {:?} has cost {:?}", - transaction, self.transaction_cost - ); - &self.transaction_cost + debug!("transaction {:?} has cost {:?}", transaction, tx_cost); + tx_cost } pub fn upsert_instruction_cost( @@ -146,7 +140,8 @@ impl CostModel { } fn get_write_lock_cost( - &mut self, + &self, + tx_cost: &mut TransactionCost, transaction: &SanitizedTransaction, demote_program_write_locks: bool, ) { @@ -154,9 +149,11 @@ impl CostModel { message.account_keys_iter().enumerate().for_each(|(i, k)| { let is_writable = message.is_writable(i, demote_program_write_locks); + // TODO TAO - is pushing key really this expensive? Test it, otherwise, + // can push index if is_writable { - self.transaction_cost.writable_accounts.push(*k); - self.transaction_cost.write_lock_cost += WRITE_LOCK_UNITS; + tx_cost.writable_accounts.push(*k); + tx_cost.write_lock_cost += WRITE_LOCK_UNITS; } }); } @@ -368,7 +365,7 @@ mod tests { .try_into() .unwrap(); - let mut cost_model = CostModel::default(); + let cost_model = CostModel::default(); let tx_cost = cost_model.calculate_cost(&tx, /*demote_program_write_locks=*/ true); assert_eq!(2 + 2, tx_cost.writable_accounts.len()); assert_eq!(signer1.pubkey(), tx_cost.writable_accounts[0]); @@ -479,7 +476,7 @@ mod tests { }) } else { thread::spawn(move || { - let mut cost_model = cost_model.write().unwrap(); + let cost_model = cost_model.write().unwrap(); let tx_cost = cost_model .calculate_cost(&tx, /*demote_program_write_locks=*/ true); assert_eq!(3, tx_cost.writable_accounts.len()); diff --git a/runtime/src/cost_tracker.rs b/runtime/src/cost_tracker.rs index ece85bc3d32606..5f717d423ccc6b 100644 --- a/runtime/src/cost_tracker.rs +++ b/runtime/src/cost_tracker.rs @@ -7,9 +7,7 @@ use crate::block_cost_limits::*; use crate::cost_model::TransactionCost; use crate::cost_tracker_stats::CostTrackerStats; use solana_sdk::{clock::Slot, pubkey::Pubkey, transaction::SanitizedTransaction}; -use std::{ - collections::HashMap, -}; +use std::collections::HashMap; const WRITABLE_ACCOUNTS_PER_BLOCK: usize = 512; @@ -199,7 +197,7 @@ mod tests { system_transaction, transaction::Transaction, }; - use std::{cmp, sync::Arc}; + use std::{cmp, convert::TryFrom, sync::Arc}; fn test_setup() -> (Keypair, Hash) { solana_logger::setup(); @@ -226,7 +224,7 @@ mod tests { #[test] fn test_cost_tracker_initialization() { - let testee = CostTracker::new(Arc::new(RwLock::new(CostModel::new(10, 11)))); + let testee = CostTracker::new(10, 11); assert_eq!(10, testee.account_cost_limit); assert_eq!(11, testee.block_cost_limit); assert_eq!(0, testee.cost_by_writable_accounts.len()); @@ -239,7 +237,7 @@ mod tests { let (_tx, keys, cost) = build_simple_transaction(&mint_keypair, &start_hash); // build testee to have capacity for one simple transaction - let mut testee = CostTracker::new(Arc::new(RwLock::new(CostModel::new(cost, cost)))); + let mut testee = CostTracker::new(cost, cost); assert!(testee .would_fit(&keys, &cost, &mut CostTrackerStats::default()) .is_ok()); @@ -255,10 +253,10 @@ mod tests { let (_tx2, keys2, cost2) = build_simple_transaction(&mint_keypair, &start_hash); // build testee to have capacity for two simple transactions, with same accounts - let mut testee = CostTracker::new(Arc::new(RwLock::new(CostModel::new( + let mut testee = CostTracker::new( cost1 + cost2, cost1 + cost2, - )))); + ); { assert!(testee .would_fit(&keys1, &cost1, &mut CostTrackerStats::default()) @@ -284,10 +282,10 @@ mod tests { let (_tx2, keys2, cost2) = build_simple_transaction(&second_account, &start_hash); // build testee to have capacity for two simple transactions, with same accounts - let mut testee = CostTracker::new(Arc::new(RwLock::new(CostModel::new( + let mut testee = CostTracker::new( cmp::max(cost1, cost2), cost1 + cost2, - )))); + ); { assert!(testee .would_fit(&keys1, &cost1, &mut CostTrackerStats::default()) @@ -312,10 +310,10 @@ mod tests { let (_tx2, keys2, cost2) = build_simple_transaction(&mint_keypair, &start_hash); // build testee to have capacity for two simple transactions, but not for same accounts - let mut testee = CostTracker::new(Arc::new(RwLock::new(CostModel::new( + let mut testee = CostTracker::new( cmp::min(cost1, cost2), cost1 + cost2, - )))); + ); // should have room for first transaction { assert!(testee @@ -340,10 +338,10 @@ mod tests { let (_tx2, keys2, cost2) = build_simple_transaction(&second_account, &start_hash); // build testee to have capacity for each chain, but not enough room for both transactions - let mut testee = CostTracker::new(Arc::new(RwLock::new(CostModel::new( + let mut testee = CostTracker::new( cmp::max(cost1, cost2), cost1 + cost2 - 1, - )))); + ); // should have room for first transaction { assert!(testee @@ -360,48 +358,11 @@ mod tests { } #[test] - fn test_cost_tracker_reset() { + fn test_cost_tracker_try_add_is_atomic() { let (mint_keypair, start_hash) = test_setup(); - // build two transactions with same signed account - let (_tx1, keys1, cost1) = build_simple_transaction(&mint_keypair, &start_hash); - let (_tx2, keys2, cost2) = build_simple_transaction(&mint_keypair, &start_hash); + let (tx, _keys, _cost) = build_simple_transaction(&mint_keypair, &start_hash); + let tx = SanitizedTransaction::try_from(tx).unwrap(); - // build testee to have capacity for two simple transactions, but not for same accounts - let mut testee = CostTracker::new(Arc::new(RwLock::new(CostModel::new( - cmp::min(cost1, cost2), - cost1 + cost2, - )))); - // should have room for first transaction - { - assert!(testee - .would_fit(&keys1, &cost1, &mut CostTrackerStats::default()) - .is_ok()); - testee.add_transaction(&keys1, &cost1); - assert_eq!(1, testee.cost_by_writable_accounts.len()); - assert_eq!(cost1, testee.block_cost); - } - // but no more sapce on the same chain (same signer account) - { - assert!(testee - .would_fit(&keys2, &cost2, &mut CostTrackerStats::default()) - .is_err()); - } - // reset the tracker - { - testee.reset_if_new_bank(100, &mut CostTrackerStats::default()); - assert_eq!(0, testee.cost_by_writable_accounts.len()); - assert_eq!(0, testee.block_cost); - } - //now the second transaction can be added - { - assert!(testee - .would_fit(&keys2, &cost2, &mut CostTrackerStats::default()) - .is_ok()); - } - } - - #[test] - fn test_cost_tracker_try_add_is_atomic() { let acct1 = Pubkey::new_unique(); let acct2 = Pubkey::new_unique(); let acct3 = Pubkey::new_unique(); @@ -409,10 +370,10 @@ mod tests { let account_max = cost * 2; let block_max = account_max * 3; // for three accts - let mut testee = CostTracker::new(Arc::new(RwLock::new(CostModel::new( + let mut testee = CostTracker::new( account_max, block_max, - )))); + ); // case 1: a tx writes to 3 accounts, should success, we will have: // | acct1 | $cost | @@ -426,7 +387,7 @@ mod tests { ..TransactionCost::default() }; assert!(testee - .try_add(&tx_cost, &mut CostTrackerStats::default()) + .try_add(&tx, &tx_cost, &mut CostTrackerStats::default()) .is_ok()); let stat = testee.get_stats(); assert_eq!(cost, stat.total_cost); @@ -446,7 +407,7 @@ mod tests { ..TransactionCost::default() }; assert!(testee - .try_add(&tx_cost, &mut CostTrackerStats::default()) + .try_add(&tx, &tx_cost, &mut CostTrackerStats::default()) .is_ok()); let stat = testee.get_stats(); assert_eq!(cost * 2, stat.total_cost); @@ -468,7 +429,7 @@ mod tests { ..TransactionCost::default() }; assert!(testee - .try_add(&tx_cost, &mut CostTrackerStats::default()) + .try_add(&tx, &tx_cost, &mut CostTrackerStats::default()) .is_err()); let stat = testee.get_stats(); assert_eq!(cost * 2, stat.total_cost);