CostModel::estimate_cost #3986
CostModel::estimate_cost #3986
Conversation
apfitzge
force-pushed
the
estimate_cost
branch
from
ba08fee to
3481f30
Compare
cost-model/src/cost_model.rs
Outdated
| /// | ||
| /// This assumes ALL features are activated. | ||
| /// This is intended to be used as an estimate of cost ONLY for the purposes of | ||
| /// prioritization and packing. |
There was a problem hiding this comment.
estimate_cost() essentially does same thing as calculate_cost(), but with a better interface. Both provide estimated cost for packing, for that purpose, it doesn't need a fully sanitized transaction if num_write_locks is known. How about sunset calculate_cost()? Maybe first step: fn calculate_cost(tx, feature_set) -> u64 { Self::estimate_cost(tx, tx.program_iter(), feature_set) }?
There was a problem hiding this comment.
ps. calculate_cost_for_executed_transaction() (is for block limits enforcing) doesn't need fully sanitized transaction either, if num_write_locks is known.
Then the entire cost_model can be solely depends on StaticMeta, then "estimated cost" can itself be part of StaticMeta! wdyt?
There was a problem hiding this comment.
Yeah could do it - I guess my concern was primarily right now the number of write locks is behind a feature-gate.
Because before feature is activated we count actual number of locks.
After we count requested write locks.
So we technically need to have a fully sanitized transaction (at least to point of ALT resolution and lock demotion - which are usually our last steps) until we clean up that feature.
So it felt like a foot-gun to make that simple integer be an input for the "main" entrypoints.
I think once that feature is activated its' probably fine?
There was a problem hiding this comment.
I reworked it so that all 3 main functions: calculate_cost, calculate_cost_for_executed_transaction, estimate_cost all have a shared implementation called for the non-vote transactions.
cost-model/src/cost_model.rs
Outdated
|
|
||
| lazy_static! { | ||
| static ref ALL_FEATURES: FeatureSet = FeatureSet::all_enabled(); | ||
| }; |
There was a problem hiding this comment.
As suggested above, can let call side decided what feature_set to use.
apfitzge
force-pushed
the
estimate_cost
branch
from
168cb61 to
d40162b
Compare
cost-model/src/cost_model.rs
Outdated
|
|
||
| TransactionCost::Transaction(usage_cost_details) | ||
| } | ||
| let num_write_locks = Self::num_write_locks(transaction, feature_set); |
There was a problem hiding this comment.
Don't have to gather write locks if it's simple-vote. Could make all three public functions (calculate_cost, calculate_cost_for_executed_transaction, estimate_cost) have same
if transaction.is_simple_vote_transaction() {
return TransactionCost::SimpleVote { transaction };
}
// gather necessry stuff: num_write_locks, get_transaction_cost etc
Self::calculate_non_vote_transaction_cost(...)
}
| fn calculate_non_vote_transaction_cost<'a, Tx: StaticMeta>( | ||
| transaction: &'a Tx, | ||
| instructions: impl Iterator<Item = (&'a Pubkey, SVMInstruction<'a>)> + Clone, | ||
| num_write_locks: u64, |
There was a problem hiding this comment.
nit: this parameter stands out as 'count' while the rest of parameters are 'cost'. Maybe change it to `cost' as well?
apfitzge
force-pushed
the
estimate_cost
branch
from
d40162b to
615f225
Compare
Problem
Summary of Changes
CostModelfunctions to take instruction iterator or static metaCostModel::estimate_costFixes #