-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Fuel APIs for Store #7298
New Fuel APIs for Store #7298
Conversation
Subscribe to Label Actioncc @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:
To subscribe or unsubscribe from this label, edit the |
cd6b687
to
555e632
Compare
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
To modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
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. |
90b02f6
to
a56fba3
Compare
@alexcrichton test are passing (no rush on a review, I wont be able to make updates until Monday anyways) |
c23fc40
to
c30ab9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on what to do for the C API in that case? Do we just translate the 0 there into None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's reasonable to do for the C API as magical numbers like that are relatively common in C
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I've made this change then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on what to do for the C API in that case? Do we just translate the 0 there into None
?
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>
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 ofu64
before this change it could only return up to
i64::MAX
. This isimportant 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 bepositive 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 tryingto figure out what to do if the yield interval is
0
.