Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Remove clippy exception that is no longer needed #30289

Merged
merged 1 commit into from
Feb 14, 2023
Merged

chore: Remove clippy exception that is no longer needed #30289

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions ledger/src/blockstore_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,6 @@ fn execute_batches(

let mut minimal_tx_cost = u64::MAX;
let mut total_cost: u64 = 0;
// Allowing collect here, since it also computes the minimal tx cost, and aggregate cost.
// These two values are later used for checking if the tx_costs vector needs to be iterated over.
#[allow(clippy::needless_collect)]
Copy link
Contributor

@tao-stones tao-stones Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - maybe keep the comment? When I first looked at this code, i thought why collect here, iterator should be enough; then the comment points out it also computes local variables. So in that sense, it's useful; (or refactor Ln 339-340 out)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, my thinking was that with your changes in place, we do explicitly use the tx_costs later on. So even tho we are updating variables as we iterate through, we are going to explicitly use the collected vector as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, think my code below can use mapped iterators, doesn't really need vec<Tx_cost> being collected first. But I can see now that comments can be redundant here. No problem to remove it. Can do refactor in other time.

Copy link
Contributor Author

@steviez steviez Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CostModel::calculate_cost() returns a TransactionCost that has non-trivial members (namely that Vec<Pubkey>).

solana/runtime/src/cost_model.rs

Lines 31 to 40 in 2ba7327

pub struct TransactionCost {
pub writable_accounts: Vec<Pubkey>,
pub signature_cost: u64,
pub write_lock_cost: u64,
pub data_bytes_cost: u64,
pub builtins_execution_cost: u64,
pub bpf_execution_cost: u64,
pub account_data_size: u64,
pub is_simple_vote: bool,
}

Once your newly added feature becomes active, we might iterate through tx_costs (the Vec that results from the collect()) twice. Here

solana/ledger/src/blockstore_processor.rs

Line 350 in 2ba7327

for tx_cost in &tx_costs {

and here:

solana/ledger/src/blockstore_processor.rs

Lines 364 to 365 in 2ba7327

tx_costs
.into_iter()

If we skip the collect(), we would have to calculate / allocate a TransactionCost during these two iterations, something like the below right?

sanitized_txs.iter().map(|tx|
    let tx_cost = CostModel::calculate_cost(tx, &bank.feature_set);
)

The implementation of CostModel::calculate_cost() looks non-trivial enough on the surface that it seems avoiding this might be preferable.

In case I'm mistaken and there would be merit to the refactor, agreed that separating into separate PR is the move so I'm going to proceed with this one.

let tx_costs = sanitized_txs
.iter()
.map(|tx| {
Expand Down