fuel: Defer fuel consumption for bulk operations - #14213
Conversation
For bulk operations and table/memory grows, wait until the operation has succeeded to consume fuel. This prevents large operations that fail from consuming fuel for work that doesn't actually occur.
| i32.const 0 | ||
| i32.const 65536 | ||
| memory.copy | ||
| (loop) |
There was a problem hiding this comment.
Since we're now consuming the fuel after the fuel check, a bunch of these tests need an additional fuel check inserted at the end to pass. The empty loop block does that.
There was a problem hiding this comment.
Could the fuel check move to after the operation instead of before the operation to handle this? That way while the fuel running out would be retroactively discovered it'd basically be the same thing and wouldn't need extra updates
There was a problem hiding this comment.
Yeah we can do that. I think it'll require changing some other tests which assert that we don't do a bulk operation if fuel is at 0 just before the op, but that seems fine to me.
There was a problem hiding this comment.
Yeah that seems ok to me, a lot of the fuel tests are about precise behavior and it's ok to tweak them if the behavior is slightly adjusted as well
|
|
||
| #[wasmtime_test(wasm_features(memory64), strategies(not(Winch)))] | ||
| #[cfg_attr(miri, ignore)] | ||
| fn memory64_variable_operator_cost_saturates(config: &mut Config) -> Result<()> { |
There was a problem hiding this comment.
I'm not sure what to do with these saturating fuel tests. Now that we only consume the fuel on a successful operation we shouldn't be able to actually hit the saturation.
| // Skip explicit fuel/epoch checks for operations which are | ||
| // subjectively, and statically, considered cheap. | ||
| // subjectively, and statically, considered cheap and consume the fuel | ||
| // now instead of waiting to see if the operation succeeds. | ||
| const SMALL_BULK_OP_COST: i64 = 128; | ||
| if let Some(units) = const_units | ||
| && let Some(cost) = units.checked_mul(i64::from(cost_per_unit)) | ||
| && cost <= SMALL_BULK_OP_COST | ||
| { | ||
| if should_consume_fuel { | ||
| self.fuel_consumed = self.fuel_consumed.saturating_add(cost); | ||
| } | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
One thing that's a little weird now is that we use fuel to decide if the operation is small and then that determines if we skip the epoch check after the operation. So if a user set the fuel cost_per_unit to 0 and ran with epochs then we wouldn't run the epoch check after a large bulk operation. Not sure if that needs to be fixed - alternatives would be to always do the epoch check, or have distinct conditions for a "small operation" when epochs are enabled.
There was a problem hiding this comment.
That to me feels like an obscure enough edge case that it's ok. If an embedder says these operations have 0 cost then skipping the epoch check almost seems faithful to that since if it's free why bother checking epochs in more places.
| else { | ||
| return Ok(()); | ||
| }; | ||
| debug_assert!(self.tunables.consume_fuel && cost_per_unit > 0); |
There was a problem hiding this comment.
I was thinking this debug assert was suspicious because we should able to get here with just epochs, and then I saw CI and looks like it additionally discovered that.
There was a problem hiding this comment.
Yeah sorry, just fixed it.
| // Skip explicit fuel/epoch checks for operations which are | ||
| // subjectively, and statically, considered cheap. | ||
| // subjectively, and statically, considered cheap and consume the fuel | ||
| // now instead of waiting to see if the operation succeeds. | ||
| const SMALL_BULK_OP_COST: i64 = 128; | ||
| if let Some(units) = const_units | ||
| && let Some(cost) = units.checked_mul(i64::from(cost_per_unit)) | ||
| && cost <= SMALL_BULK_OP_COST | ||
| { | ||
| if should_consume_fuel { | ||
| self.fuel_consumed = self.fuel_consumed.saturating_add(cost); | ||
| } | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
That to me feels like an obscure enough edge case that it's ok. If an embedder says these operations have 0 cost then skipping the epoch check almost seems faithful to that since if it's free why bother checking epochs in more places.
| fn variable_operator_cost_failed_growth(config: &mut Config) -> Result<()> { | ||
| fn variable_operator_cost_charged_only_on_success(config: &mut Config) -> Result<()> { |
There was a problem hiding this comment.
It seems useful to test a number of operators here such as the growth ones and memory.fill, and in retrospect I think all of this test could be added to fuel.wast perhaps? Trapping behavior can't be added there but it seems reasonable to me to skip testing the trapping behavior since the semantics there are sort of inherited from general translation infrastructure which may already be tested too.
| /// Runs a wasm which grows a memory and a table each by `size` and returns the | ||
| /// fuel consumed. The memory and table max sizes will be set so that the grow | ||
| /// succeeds or fails as determined by the `succeeds` parameter. | ||
| fn grow_fuel_consumed(config: &mut Config, size: GrowSize, succeeds: bool) -> Result<u64> { |
There was a problem hiding this comment.
Could the various permutations of this test all get added to fuel.wast instead? Naively it seems like that'd work, but I might be missing something too
| /// operations would consume fuel. | ||
| #[wasmtime_test(wasm_features(bulk_memory, memory64), strategies(not(Winch)))] | ||
| #[cfg_attr(miri, ignore)] | ||
| fn oob_memory_fill_does_not_consume_fuel(config: &mut Config) -> Result<()> { |
There was a problem hiding this comment.
Is there any particular reason to single out memory.fill here vs other operations? If this is specifically a regression test for one of the original failures I think it's ok to skip this test since it'll be covered by fuzzing
2 participants
For bulk operations and memory/table grows, defer consuming fuel until the operation has succeed.
This fixes #14161.
For small statically known sizes, we immediately consume fuel, but for larger or dynamic sizes we insert a fuel/epoch check before the operation and then only consume the variable fuel amount after.