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.

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

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Feb 13, 2023

Problem

We use the Vec that results from .collect(), so the exception to allow a needless collect is no longer appropriate.

Summary of Changes

Remove outdated clippy exception; noticed this while looking at another review.

We use the Vec that results from .collect(), so the exception to allow a
needless collect is no longer appropriate.
@steviez steviez requested a review from tao-stones February 13, 2023 19:38
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm - thanks for paying detailed attentions. Just a nit about comment.

@@ -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

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

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>).

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

for tx_cost in &tx_costs {

and here:
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.

@steviez steviez merged commit c617c34 into solana-labs:master Feb 14, 2023
@steviez steviez deleted the cleanup branch February 14, 2023 06:07
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Mar 12, 2023
…30289)

We use the Vec that results from .collect(), so the exception to allow a
needless collect is no longer appropriate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants