Address review comment: extract transaction was_executed status to avoid cloning execution_results
This commit is contained in:
parent
094da35b91
commit
23d365d02f
|
@ -117,8 +117,8 @@ pub struct ExecuteAndCommitTransactionsOutput {
|
|||
// how many such transactions were committed
|
||||
commit_transactions_result: Result<(), PohRecorderError>,
|
||||
execute_and_commit_timings: LeaderExecuteAndCommitTimings,
|
||||
// Transaction execution detail results
|
||||
execution_results: Vec<TransactionExecutionResult>,
|
||||
// True if transaction was-executed()
|
||||
transactions_executed_status: Vec<bool>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
|
@ -1206,6 +1206,11 @@ impl BankingStage {
|
|||
..
|
||||
} = load_and_execute_transactions_output;
|
||||
|
||||
let transactions_executed_status = execution_results
|
||||
.iter()
|
||||
.map(|execution_result| execution_result.was_executed())
|
||||
.collect();
|
||||
|
||||
let (freeze_lock, freeze_lock_time) =
|
||||
Measure::this(|_| bank.freeze_lock(), (), "freeze_lock");
|
||||
execute_and_commit_timings.freeze_lock_us = freeze_lock_time.as_us();
|
||||
|
@ -1249,7 +1254,7 @@ impl BankingStage {
|
|||
retryable_transaction_indexes,
|
||||
commit_transactions_result: Err(e),
|
||||
execute_and_commit_timings,
|
||||
execution_results,
|
||||
transactions_executed_status,
|
||||
};
|
||||
}
|
||||
|
||||
|
@ -1264,7 +1269,7 @@ impl BankingStage {
|
|||
bank.commit_transactions(
|
||||
sanitized_txs,
|
||||
&mut loaded_transactions,
|
||||
execution_results.clone(),
|
||||
execution_results,
|
||||
executed_transactions_count as u64,
|
||||
executed_transactions_count
|
||||
.saturating_sub(executed_with_successful_result_count)
|
||||
|
@ -1336,7 +1341,7 @@ impl BankingStage {
|
|||
retryable_transaction_indexes,
|
||||
commit_transactions_result: Ok(()),
|
||||
execute_and_commit_timings,
|
||||
execution_results,
|
||||
transactions_executed_status,
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -1393,7 +1398,7 @@ impl BankingStage {
|
|||
let ExecuteAndCommitTransactionsOutput {
|
||||
ref mut retryable_transaction_indexes,
|
||||
ref execute_and_commit_timings,
|
||||
ref execution_results,
|
||||
ref transactions_executed_status,
|
||||
..
|
||||
} = execute_and_commit_transactions_output;
|
||||
|
||||
|
@ -1403,7 +1408,7 @@ impl BankingStage {
|
|||
QosService::update_or_remove_transaction_costs(
|
||||
transaction_costs.iter(),
|
||||
transactions_qos_results.iter(),
|
||||
execution_results.iter(),
|
||||
transactions_executed_status.iter(),
|
||||
bank,
|
||||
);
|
||||
|
||||
|
|
|
@ -7,7 +7,7 @@ use {
|
|||
crossbeam_channel::{unbounded, Receiver, Sender},
|
||||
solana_measure::measure::Measure,
|
||||
solana_runtime::{
|
||||
bank::{Bank, TransactionExecutionResult},
|
||||
bank::Bank,
|
||||
cost_model::{CostModel, TransactionCost},
|
||||
cost_tracker::CostTrackerError,
|
||||
},
|
||||
|
@ -177,18 +177,18 @@ impl QosService {
|
|||
pub fn update_or_remove_transaction_costs<'a>(
|
||||
transaction_costs: impl Iterator<Item = &'a TransactionCost>,
|
||||
transaction_qos_results: impl Iterator<Item = &'a transaction::Result<()>>,
|
||||
execution_results: impl Iterator<Item = &'a TransactionExecutionResult>,
|
||||
transaction_executed_status: impl Iterator<Item = &'a bool>,
|
||||
bank: &Arc<Bank>,
|
||||
) {
|
||||
let mut cost_tracker = bank.write_cost_tracker().unwrap();
|
||||
transaction_costs
|
||||
.zip(transaction_qos_results)
|
||||
.zip(execution_results)
|
||||
.for_each(|((tx_cost, qos_inclusion_result), execution_result)| {
|
||||
.zip(transaction_executed_status)
|
||||
.for_each(|((tx_cost, qos_inclusion_result), executed_status)| {
|
||||
// Only transactions that the qos service included have to be
|
||||
// checked for remove or update/commit
|
||||
if qos_inclusion_result.is_ok() {
|
||||
if execution_result.was_executed() {
|
||||
if *executed_status {
|
||||
cost_tracker.update_execution_cost(tx_cost, None);
|
||||
} else {
|
||||
cost_tracker.remove(tx_cost);
|
||||
|
@ -494,10 +494,7 @@ mod tests {
|
|||
use {
|
||||
super::*,
|
||||
itertools::Itertools,
|
||||
solana_runtime::{
|
||||
bank::TransactionExecutionDetails,
|
||||
genesis_utils::{create_genesis_config, GenesisConfigInfo},
|
||||
},
|
||||
solana_runtime::genesis_utils::{create_genesis_config, GenesisConfigInfo},
|
||||
solana_sdk::{
|
||||
hash::Hash,
|
||||
signature::{Keypair, Signer},
|
||||
|
@ -626,21 +623,11 @@ mod tests {
|
|||
total_txs_costs,
|
||||
bank.read_cost_tracker().unwrap().block_cost()
|
||||
);
|
||||
let execution_results: Vec<TransactionExecutionResult> = (0..transaction_count)
|
||||
.map(|_| {
|
||||
TransactionExecutionResult::Executed(TransactionExecutionDetails {
|
||||
status: Ok(()),
|
||||
log_messages: None,
|
||||
inner_instructions: None,
|
||||
durable_nonce_fee: None,
|
||||
return_data: None,
|
||||
})
|
||||
})
|
||||
.collect();
|
||||
let executed_status: Vec<bool> = (0..transaction_count).map(|_| true).collect();
|
||||
QosService::update_or_remove_transaction_costs(
|
||||
txs_costs.iter(),
|
||||
qos_results.iter(),
|
||||
execution_results.iter(),
|
||||
executed_status.iter(),
|
||||
&bank,
|
||||
);
|
||||
assert_eq!(
|
||||
|
@ -677,13 +664,11 @@ mod tests {
|
|||
let txs_costs = qos_service.compute_transaction_costs(txs.iter());
|
||||
let (qos_results, _num_included) =
|
||||
qos_service.select_transactions_per_cost(txs.iter(), txs_costs.iter(), &bank);
|
||||
let execution_results: Vec<TransactionExecutionResult> = (0..transaction_count)
|
||||
.map(|_| TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound))
|
||||
.collect();
|
||||
let executed_status: Vec<bool> = (0..transaction_count).map(|_| false).collect();
|
||||
QosService::update_or_remove_transaction_costs(
|
||||
txs_costs.iter(),
|
||||
qos_results.iter(),
|
||||
execution_results.iter(),
|
||||
executed_status.iter(),
|
||||
&bank,
|
||||
);
|
||||
assert_eq!(0, bank.read_cost_tracker().unwrap().block_cost());
|
||||
|
@ -708,31 +693,17 @@ mod tests {
|
|||
.map(|_| transfer_tx.clone())
|
||||
.collect();
|
||||
|
||||
// assert only successfully executed tx_costs are applied cost_tracker
|
||||
// assert only executed tx_costs are applied cost_tracker
|
||||
{
|
||||
let qos_service = QosService::new(Arc::new(RwLock::new(CostModel::default())), 1);
|
||||
let txs_costs = qos_service.compute_transaction_costs(txs.iter());
|
||||
let (qos_results, _num_included) =
|
||||
qos_service.select_transactions_per_cost(txs.iter(), txs_costs.iter(), &bank);
|
||||
let execution_results: Vec<TransactionExecutionResult> = (0..transaction_count)
|
||||
.map(|n| {
|
||||
if n < 1 {
|
||||
TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound)
|
||||
} else {
|
||||
TransactionExecutionResult::Executed(TransactionExecutionDetails {
|
||||
status: Ok(()),
|
||||
log_messages: None,
|
||||
inner_instructions: None,
|
||||
durable_nonce_fee: None,
|
||||
return_data: None,
|
||||
})
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
let executed_status: Vec<bool> = (0..transaction_count).map(|n| n != 0).collect();
|
||||
QosService::update_or_remove_transaction_costs(
|
||||
txs_costs.iter(),
|
||||
qos_results.iter(),
|
||||
execution_results.iter(),
|
||||
executed_status.iter(),
|
||||
&bank,
|
||||
);
|
||||
let expected_committed_units: u64 = txs_costs
|
||||
|
|
Loading…
Reference in New Issue