Stream: git-wasmtime

Topic: wasmtime / PR #7298 New Fuel APIs for Store

view this post on Zulip Wasmtime GitHub notifications bot (Oct 19 2023 at 21:23):

rockwotj requested pchickey for a review on PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 19 2023 at 21:23):

rockwotj opened PR #7298 from rockwotj:fuel2 to bytecodealliance:main:

Fixes: #7255

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

I also took a stab in the second commit in being able to support the full range of u64 so that you can set_fuel(u64::MAX) then get_fuel() == u64::MAX. This makes accounting externally less tricky since we're removing that functionality from wasmtime directly.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 19 2023 at 21:23):

rockwotj requested wasmtime-core-reviewers for a review on PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 19 2023 at 21:26):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 19 2023 at 21:28):

rockwotj edited PR #7298:

Fixes: #7255

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

I also took a stab in the second commit in being able to support the full range of u64 so that you can set_fuel(u64::MAX) then get_fuel() == u64::MAX. This makes accounting externally less tricky since we're removing that functionality from wasmtime directly.

prtest:full

view this post on Zulip Wasmtime GitHub notifications bot (Oct 19 2023 at 21:29):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2023 at 15:04):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2023 at 15:07):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2023 at 15:13):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2023 at 15:31):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2023 at 16:19):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2023 at 19:09):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2023 at 20:34):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 20 2023 at 22:03):

alexcrichton requested alexcrichton for a review on PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2023 at 00:32):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2023 at 00:32):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2023 at 00:33):

rockwotj edited PR #7298:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2023 at 00:38):

rockwotj submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2023 at 00:38):

rockwotj submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2023 at 00:38):

rockwotj created PR review comment:

I wanted to make sure to write unit tests for the methods standalone from the rest to ensure I got all the overflow cases correct, so hence pulling these methods out as standalone so they can be tested directly. I don't know if this is a "normal" pattern or not, but it did help me catch a couple of bugs.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2023 at 00:47):

rockwotj edited PR #7298:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 00:39):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 01:12):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 15:09):

alexcrichton submitted PR review:

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!)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 15:09):

alexcrichton submitted PR review:

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!)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 15:09):

alexcrichton created PR review comment:

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)?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 15:09):

alexcrichton created PR review comment:

Nah while this isn't common right now I think this is a good idea, so thanks for adding these tests!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 15:09):

alexcrichton created PR review comment:

Reader over this I realize that the term "consumed" here is inherited from the StoreLimits but in isolation I think it's somewhat confusing because it's not actually consumed fuel, it's actually "negating this gives you some remaining fuel".

I'm curious, do you have an idea of a better name for this? Not a blocker for the PR of course, but if you've got an idea for a better name it'd definitely be ok to rename here.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 17:11):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 17:12):

rockwotj submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 17:12):

rockwotj created PR review comment:

I've been thinking about this in terms of "active fuel" or "injected fuel". I'll use the term "injected" unless you prefer "active"

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 17:12):

rockwotj submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 17:12):

rockwotj created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 17:13):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 17:16):

rockwotj requested alexcrichton for a review on PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 18:40):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 18:41):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 18:41):

alexcrichton created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 18:57):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 18:58):

rockwotj submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 18:58):

rockwotj created PR review comment:

Okay I've made this change then.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 18:58):

rockwotj updated PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 18:59):

rockwotj requested alexcrichton for a review on PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 19:03):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 19:03):

alexcrichton has enabled auto merge for PR #7298.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 20:43):

alexcrichton merged PR #7298.

Last updated: Nov 22 2024 at 16:03 UTC