New Fuel APIs for Store #7298

rockwotj · 2023-10-19T21:23:21Z

In an effort to simplify the many fuel related APIs, simplify the
interface here to a single counter with get and set methods.
Additionally the async yield is reduced to an interval of the total fuel
instead of injecting fuel, so it's easy to still reason about how much
fuel is left even with yielding turned on.

Internally this works by keeping two counters - one the VM uses to
increment towards 0 for fuel, the other to track how much is in
"reserve". Then when we're out of gas, we pull from the reserve to
refuel and continue. We use the reserve in two cases: one for overflow
of the fuel (which is an i64 and the API expresses fuel as u64) and the
other for async yieling, which then the yield interval acts as a cap to
how much we can refuel with.

This also means that get_fuel can return the full range of u64
before this change it could only return up to i64::MAX. This is
important because this PR is removing the functionality to track fuel
consumption, and this makes the API less error prone for embedders to
track consumption themselves.

Careful to note that the VM counter that is stored as i64 can be
positive if an instruction "costs" multiple units of fuel when the fuel
ran out.

Fixes: #7255

I've implemented the APIs as documented in the strawman from the issue,
but I used Option<NonZeroU64> for the yield interval instead of trying
to figure out what to do if the yield interval is 0.

github-actions · 2023-10-19T21:44:44Z

Subscribe to Label Action

cc @fitzgen, @peterhuene

This issue or pull request has been labeled: "fuzzing", "wasmtime:api", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing
peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2023-10-20T15:45:06Z

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

If you added a new Config method, you wrote extensive documentation for
it.

Our documentation should be of the following form:

Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

# Example

Optional example here.

If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in wasmtime_fuzzing::Config (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See our docs on fuzzing for more details.
If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

alexcrichton · 2023-10-20T22:03:28Z

I'll take a look once CI is green, so feel free to ping me when that comes up. I'll check in every now and then as well.

crates/wasmtime/src/store.rs

rockwotj · 2023-10-21T01:13:25Z

@alexcrichton test are passing (no rush on a review, I wont be able to make updates until Monday anyways)

alexcrichton

Wow I really like how this all turned out, thank you for pushing on this!

I've got a few cosmetic-related concerns but otherwise I think this is good to go. If you're up for it as well it might be good to do a once-over of fuel-related documentation not only on Store but also Config and perhaps other locations since this is a somewhat significant departure from the prior model (but well worth it). For example running out of fuel now unconditionally panics and that cannot be configured, which may be lingering in a few locations (and if not then nothing to do yay!)

crates/wasmtime/src/store.rs

alexcrichton · 2023-10-23T15:07:47Z

crates/wasmtime/src/store.rs

-    pub fn out_of_fuel_async_yield(&mut self, injection_count: u64, fuel_to_inject: u64) {
-        self.inner
-            .out_of_fuel_async_yield(injection_count, fuel_to_inject)
+    pub fn fuel_async_yield_interval(&mut self, interval: Option<NonZeroU64>) -> Result<()> {


While I think it definitely makes sense to store this as Option<NonZeroU64> taking that as a function parameter here may be a bit overkill in the sense that it's somewhat unergonomic to call. Could this instead take Option<u64> and return an error for Some(0)?

Thoughts on what to do for the C API in that case? Do we just translate the 0 there into None?

Yeah I think that's reasonable to do for the C API as magical numbers like that are relatively common in C

Okay I've made this change then.

rockwotj

If you're up for it as well it might be good to do a once-over of fuel-related documentation not only on Store but also Config and perhaps other locations since this is a somewhat significant departure from the prior model (but well worth it). For example running out of fuel now unconditionally panics and that cannot be configured, which may be lingering in a few locations (and if not then nothing to do yay!)

I found one place in Config::consume_fuel so nice suggestion! A quick glance over the project via git grep fuel did not show any other usages.

crates/wasmtime/src/store.rs

rockwotj · 2023-10-23T16:55:04Z

crates/wasmtime/src/store.rs

-    pub fn out_of_fuel_async_yield(&mut self, injection_count: u64, fuel_to_inject: u64) {
-        self.inner
-            .out_of_fuel_async_yield(injection_count, fuel_to_inject)
+    pub fn fuel_async_yield_interval(&mut self, interval: Option<NonZeroU64>) -> Result<()> {


Thoughts on what to do for the C API in that case? Do we just translate the 0 there into None?

alexcrichton · 2023-10-23T18:41:15Z

oops sorry queued up for merge a bit too soon there

In an effort to simplify the many fuel related APIs, simplify the interface here to a single counter with get and set methods. Additionally the async yield is reduced to an interval of the total fuel instead of injecting fuel, so it's easy to still reason about how much fuel is left even with yielding turned on. Internally this works by keeping two counters - one the VM uses to increment towards 0 for fuel, the other to track how much is in "reserve". Then when we're out of gas, we pull from the reserve to refuel and continue. We use the reserve in two cases: one for overflow of the fuel (which is an i64 and the API expresses fuel as u64) and the other for async yieling, which then the yield interval acts as a cap to how much we can refuel with. This also means that `get_fuel` can return the full range of `u64` before this change it could only return up to `i64::MAX`. This is important because this PR is removing the functionality to track fuel consumption, and this makes the API less error prone for embedders to track consumption themselves. Careful to note that the VM counter that is stored as `i64` can be positive if an instruction "costs" multiple units of fuel when the fuel ran out. prtest:full Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>

.

. (#281)

rockwotj requested a review from a team as a code owner October 19, 2023 21:23

rockwotj requested review from pchickey and removed request for a team October 19, 2023 21:23

rockwotj force-pushed the fuel2 branch from 24192a6 to 553d5f2 Compare October 19, 2023 21:26

github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels Oct 19, 2023

rockwotj force-pushed the fuel2 branch 2 times, most recently from cd6b687 to 555e632 Compare October 20, 2023 15:13

github-actions bot added the wasmtime:config Issues related to the configuration of Wasmtime label Oct 20, 2023

rockwotj force-pushed the fuel2 branch from 8932664 to 7747745 Compare October 20, 2023 20:34

rockwotj mentioned this pull request Oct 20, 2023

Tracking issue Data Transforms + Wasm subsystems redpanda-data/redpanda#14335

Closed

alexcrichton requested review from alexcrichton and removed request for pchickey October 20, 2023 22:03

rockwotj force-pushed the fuel2 branch 2 times, most recently from 90b02f6 to a56fba3 Compare October 21, 2023 00:32

rockwotj commented Oct 21, 2023

View reviewed changes

crates/wasmtime/src/store.rs Show resolved Hide resolved

rockwotj force-pushed the fuel2 branch 2 times, most recently from c23fc40 to c30ab9b Compare October 23, 2023 01:12

alexcrichton reviewed Oct 23, 2023

View reviewed changes

rockwotj force-pushed the fuel2 branch from c30ab9b to 8e700aa Compare October 23, 2023 17:11

rockwotj commented Oct 23, 2023

View reviewed changes

rockwotj force-pushed the fuel2 branch from 8e700aa to 8a66521 Compare October 23, 2023 17:13

rockwotj requested a review from alexcrichton October 23, 2023 17:16

alexcrichton approved these changes Oct 23, 2023

View reviewed changes

alexcrichton added this pull request to the merge queue Oct 23, 2023

alexcrichton removed this pull request from the merge queue due to a manual request Oct 23, 2023

rockwotj force-pushed the fuel2 branch from 8a66521 to 7510c0c Compare October 23, 2023 18:57

rockwotj force-pushed the fuel2 branch from 7510c0c to eac17cd Compare October 23, 2023 18:58

rockwotj requested a review from alexcrichton October 23, 2023 18:59

alexcrichton approved these changes Oct 23, 2023

View reviewed changes

alexcrichton enabled auto-merge October 23, 2023 19:03

alexcrichton added this pull request to the merge queue Oct 23, 2023

Merged via the queue into bytecodealliance:main with commit 85c0a2d Oct 23, 2023
40 checks passed

rockwotj deleted the fuel2 branch October 24, 2023 00:10

kpreisser added a commit to kpreisser/wasmtime-dotnet that referenced this pull request Oct 24, 2023

Update to the new fuel APIs introduced in bytecodealliance/wasmtime#7298

10960c4

.

kpreisser mentioned this pull request Oct 24, 2023

Update to the new fuel APIs bytecodealliance/wasmtime-dotnet#281

Merged

peterhuene pushed a commit to bytecodealliance/wasmtime-dotnet that referenced this pull request Oct 24, 2023

Update to the new fuel APIs introduced in bytecodealliance/wasmtime#7298

9fbad84

. (#281)

alexcrichton mentioned this pull request Oct 25, 2023

Add Store::set_fuel and Store::fuel_remaining #5220

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Fuel APIs for Store #7298

New Fuel APIs for Store #7298

rockwotj commented Oct 19, 2023 •

edited

Loading

github-actions bot commented Oct 19, 2023

github-actions bot commented Oct 20, 2023

alexcrichton commented Oct 20, 2023

rockwotj commented Oct 21, 2023

alexcrichton left a comment

alexcrichton Oct 23, 2023

rockwotj Oct 23, 2023

alexcrichton Oct 23, 2023

rockwotj Oct 23, 2023

rockwotj left a comment

rockwotj Oct 23, 2023

alexcrichton commented Oct 23, 2023

New Fuel APIs for Store #7298

New Fuel APIs for Store #7298

Conversation

rockwotj commented Oct 19, 2023 • edited Loading

github-actions bot commented Oct 19, 2023

Subscribe to Label Action

github-actions bot commented Oct 20, 2023

Label Messager: wasmtime:config

alexcrichton commented Oct 20, 2023

rockwotj commented Oct 21, 2023

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Oct 23, 2023

Choose a reason for hiding this comment

rockwotj Oct 23, 2023

Choose a reason for hiding this comment

alexcrichton Oct 23, 2023

Choose a reason for hiding this comment

rockwotj Oct 23, 2023

Choose a reason for hiding this comment

rockwotj left a comment

Choose a reason for hiding this comment

rockwotj Oct 23, 2023

Choose a reason for hiding this comment

alexcrichton commented Oct 23, 2023

rockwotj commented Oct 19, 2023 •

edited

Loading