[benchmark] Make gas metering conditional #15901
Conversation
⏱️ 10m total CI duration on this PR
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
georgemitenkov
requested review from
vineethk,
runtian-zhou,
vgao1996,
gelash and
igor-aptos
georgemitenkov
requested review from
davidiw,
wrwg and
zekun000
as code owners
| @@ -7,7 +7,11 @@ | |||
| mod algebra; | |||
| mod meter; | |||
| mod traits; | |||
| #[cfg(feature = "benchmark")] | |||
There was a problem hiding this comment.
nit: we have a few places where we use the "bench" feature already, shall we follow the "convention"?
| static METER_GAS: OnceCell<bool> = OnceCell::new(); | ||
|
|
||
| /// If benchmarking, conditionally enable unmetered or metered executions. | ||
| pub fn enable_gas_metering(enable: bool) { | ||
| if cfg!(feature = "benchmark") { | ||
| METER_GAS.set(enable).ok(); | ||
| } | ||
| } | ||
|
|
||
| /// Returns whether the gas needs to be metered. When benchmarking, it is possible to configure it | ||
| /// to be turned off. | ||
| pub fn is_gas_metering_enabled() -> bool { | ||
| if cfg!(feature = "benchmark") { | ||
| METER_GAS.get().cloned().unwrap_or(true) | ||
| } else { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
Agreed that the pattern here offers some protection, but I still feel like this is quite hacky
- Cargo feature flags, by their design, are additive -- if any crate in your dependency tree has a flag enabled then it is enabled for the whole tree. Based on this I find it troublesome we try to use it to gate more stuff.
- I get that
METER_GAScan only set once, but still, it being a global variable breaks a convenient property of (most) Rust code -- the ability to understand side effects with only knowledge from the current scope.- One previous example is the warm VM cache -- I once spent hours debugging the VM, trying to figure out why code didn't result in the changes I expected.
- I fear that if we add more global switches to the VM it will be harder and harder to understand and control the VM behavior.
Is it possible to pass this flag in as a regular parameter? This way we let each call site control how gas should be configured.
There was a problem hiding this comment.
Agree! Tried to encapsulate, but the issue is that otherwise I need to pass meter_enabled: bool config somewhere all the way from block executor. It can be part of config, but this seems irrelevant. Do you think it is better to pass a flag all the way to single transaction?
One other thing I was trying to do is that feature ensures at compile time this is a noop, so prod performance is not impacted in any way (thogh maybe difference is marginal?)
Re global - we already do this for paranoid checks, and this is not as bad as the warm cache, but 100% agree.
Let me try the paramter, and see how easy/ugly it gets so we can compare and find the best option👍
| let vm_output = discarded_output(vm_status.status_code()); | ||
| (vm_status, vm_output) | ||
| }, | ||
| if cfg!(feature = "benchmark") && !is_gas_metering_enabled() { |
There was a problem hiding this comment.
I'm generally a bit concerned whether we could have some misconfiguration and causes this feature to go public. I would suggest at least in the node initialization logic check that the gas is indeed enabled with benchmark flag set to false in this case?
There was a problem hiding this comment.
+1, hence I used cargo feature here. Good point about the node config, do you have a code pointer?
I think we can also check if !cfg!(feature = "benchmark") is always metered for extra safety.
There was a problem hiding this comment.
aptos-core/aptos-node/src/main.rs
Line 16 in e72524a
I believe victor also had a similar check here.
Description
UnmeteredGasAlgebraandAptosUnmeteredGasMeter. We haveUnmeteredGasMeterdefined in Move repository, but that one has no access to algebra, so using this seems better."benchmark") + defaults to true, so there should be no overhead or danger for prod builds, and benchmarking should be representative enough.How Has This Been Tested?
Manual
Key Areas to Review
Checked unmetered meter is not used in prod.
Type of Change
Which Components or Systems Does This Change Impact?
Checklist