feat: allow computing a subset of costs in the estimator #4640
Conversation
matklad
force-pushed
the
matklad/cost-subset
branch
from
cad914b
to
47386de
Compare
matklad
force-pushed
the
matklad/cost-subset
branch
3 times, most recently
from
3d15776
to
369ccbd
Compare
You can now pass something like --metrics-to-measure Receipt,data_receipt_10b_1000,data_receipt_base_10b_1000 to calculate only those metrics. Note that metrics != costs. The end goal is to do this for actual costs, as outlined in near#4633 (comment) The current approach is an intermediate step in that direction. In particular, it allows to debug estimator refactors much faster. Test Plan --------- Manually run estimator to smoke test that this works, use a tool ([SSR](https://rust-analyzer.github.io/manual.html#structural-search-and-replace)) to change extraction logic without introducing typos.
matklad
force-pushed
the
matklad/cost-subset
branch
from
369ccbd
to
a2536ac
Compare
| } | ||
|
|
||
| impl Config { | ||
| pub(crate) fn should_skip(&self, metric: Metric) -> bool { |
There was a problem hiding this comment.
Extracted this to a function, because there are two places where we check this. This is a smell, ideally we'd only check this once. However, the current curious pattern of passing in and returning a testbed prevents doing that without larger changes. I opted into not doing a larger refactor for testbed resuse logic -- I hope that in #4633 I just rewrite that logic entirely.
Luckily, & and && are equivalent enough for bool for this to not matter
| @@ -294,7 +294,7 @@ pub fn run(mut config: Config, only_compile: bool) -> CostTable { | |||
| let mut f = || { | |||
| let account_idx = loop { | |||
| let x = rand::thread_rng().gen::<usize>() % config.active_accounts; | |||
| if !deleted_accounts.contains(&x) & &!beneficiaries.contains(&x) { | |||
| if !deleted_accounts.contains(&x) && !beneficiaries.contains(&x) { | |||
There was a problem hiding this comment.
Drive-by fix, but a fun one!
| } | ||
|
|
||
| fn extract_max(&mut self, metric1: Metric, metric2: Metric, ext_cost: ExtCosts) { | ||
| if let Some(value1) = self.extract_value(metric1, ext_cost) { |
There was a problem hiding this comment.
Shalln't we report error if one is present and another is not?
There was a problem hiding this comment.
I think it's fine to not report the error, as just we won't compute the cost then (that is, we won't return a wrong cost). That's essentially the same behavior we have for other complex costs computed out of several metrics -- if you omit the metric, the whole cost is omitted.
The right behavior here is to just figure out what you need to compute to get the cost, and compute just that. That's a larger refactor that is being done #4733. The goal here is primaraly to be able to quickly test if #4733 returns roughly the same results.
| @@ -200,6 +215,12 @@ where | |||
| Arc::new(Mutex::new(RuntimeTestbed::from_state_dump(&config.state_dump_path))) | |||
| } | |||
| }; | |||
|
|
|||
| if config.should_skip(metric) { | |||
| println!("skipped"); | |||
You can now pass something like
--metrics-to-measure Receipt,data_receipt_10b_1000,data_receipt_base_10b_1000
to calculate only those metrics. Note that metrics != costs. The end
goal is to do this for actual costs, as outlined in
#4633 (comment)
The current approach is an intermediate step in that direction. In
particular, it allows to debug estimator refactors much faster.
Test Plan
Manually run estimator to smoke test that this works, use a tool
(SSR)
to change extraction logic without introducing typos.