Wasmtime GitHub notifications bot (Oct 13 2023 at 21:44): Wasmtime GitHub notifications bot (Oct 13 2023 at 21:44):github-actions[bot] commented on issue #7240:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api, wasmtime:c-api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Oct 14 2023 at 03:53):alexcrichton commented on issue #7240:
One thing that's perhaps worth noting is that we have quite a litany of fuel-related methods and we seem to keep adding more over time. This isn't necessarily a problem, nor do I think this is a blocker for this PR, but I think it indicates that we haven't done a great job of picking the right abstraction related to fuel. Or perhaps they're all independently motivated but given the ad-hoc nature of the additions I suspect we can do better. The fuel methods we have right now are:
To be clear this is mostly food-for-thought. I do not wish to block this PR, and if @fitzgen approves we should merge.
Wasmtime GitHub notifications bot (Oct 14 2023 at 10:57):rockwotj commented on issue #7240:
I'm happy to come up with a better API before this PR (or make breaking changes, not sure what the policy is on that).
TBH we use fuel today and I didn't realize that consumed fuel was tracked independently of remaining fuel. Personally I probably would have pushed fuel consumed to embedders and make them track it based on how much fuel was remaining after each call into wasmtime. But I know taking away functionality is hard, but most people I've seen use fuel in the issue tracker are just attempting to prevent the engine from hogging too much CPU or getting stuck in a loop. Also I think if that functionality didn't exist you could potentially only have a set_fuel method and get_fuel method and let embedders calculate the delta. But instead there are different methods to remove fuel vs adding fuel which complicates the API vs just 'tell us how much fuel exists'.
Not sure if that makes since, but that would be the simplest primitive and API, as well as still supporting all the functionality (even if some would be pushed to embedders like tracking consumption).
Additionally, I personally find out_of_fuel_async_yield a complicated API in terms of the number of times to inject fuel. IMO the simpler API is either a method to auto reset fuel after yield to a specific value (and to trap you change out of gas behavior in the store then reset fuel to 0 and then resume the VM [as another note I'm not sure if it's valid to drop the future and still use the VM for yielding support]) or there is a yield every N fuel and you trap after fuel runs out (there is no adding fuel automatically from the API although it might do that under the hood).
If its valid to make breaking changes, I'd be happy to take a stab at a simpler fuel API based on the above in a followup PR.
Wasmtime GitHub notifications bot (Oct 14 2023 at 10:59):rockwotj edited a comment on issue #7240:
I'm happy to come up with a better API before this PR (or make breaking changes, not sure what the policy is on that).
TBH we use fuel today and I didn't realize that consumed fuel was tracked independently of remaining fuel. Personally I probably would have pushed fuel consumed to embedders and make them track it based on how much fuel was remaining after each call into wasmtime. But I know taking away functionality is hard, but most people I've seen use fuel in the issue tracker are just attempting to prevent the engine from hogging too much CPU or getting stuck in a loop. Also I think if that functionality didn't exist you could potentially only have a set_fuel method and get_fuel method and let embedders calculate the delta. But instead there are different methods to remove fuel vs adding fuel which complicates the API vs just 'tell us how much fuel exists'.
Not sure if that makes since, but that would be the simplest primitive and API, as well as still supporting all the functionality (even if some would be pushed to embedders like tracking consumption).
Additionally, I personally find out_of_fuel_async_yield a complicated API in terms of the number of times to inject fuel. IMO the simpler API is either a method to auto reset fuel after yield to a specific value (and to trap you change out of gas behavior in the store then reset fuel to 0 and then resume the VM [as another note I'm not sure if it's valid to drop the future and still use the VM for yielding support]) or there is a yield every N fuel and you trap after fuel runs out (there is no adding fuel automatically from the API although it might do that under the hood). Basically how can we get that method to only a single number input.
If its valid to make breaking changes, I'd be happy to take a stab at a simpler fuel API based on the above in a followup PR.
Wasmtime GitHub notifications bot (Oct 14 2023 at 11:08):rockwotj commented on issue #7240:
RE breaking changes I found https://github.com/bytecodealliance/wasmtime/blob/main/docs/stability-release.md which my reading of that means we would need an RFC to simplify these APIs? Not sure if this would be major or not but what I'm proposing seems to be.
Wasmtime GitHub notifications bot (Oct 14 2023 at 14:49):rockwotj commented on issue #7240:
The more I think about this the more I like the appeal of three fuel APIs in Store, I believe you could still achieve everything there is today:
get_fuel() -> Result<u64>
set_fuel(u64) -> Result<()>
fuel_async_yield_interval(u64) -> Result<()>Basically you can set and get the amount of fuel and then to enable async support and yielding we say every N fuel consumed we will yield. This also means you could get rid of the out of gas trap APIs and instead have an API to disable the yielding interval
Internally there is a current active fuel, and a reserve "tank" of fuel for restoring after a yield. The get and set operate on the combined amount of active fuel and fuel in the tank, then the current enum for out of gas behavior just becomes an optional amount of fuel to take from the tank after a yield. If the yield interval is cleared we just take all the fuel from the tank and add it to the active amount.
That feels way simpler to explain and implement. With a tighter API surface to boot. TBH i don't fully understand how fuel_adj works in the current implementation.
Wasmtime GitHub notifications bot (Oct 14 2023 at 17:14):rockwotj edited a comment on issue #7240:
The more I think about this the more I like the appeal of four fuel APIs in Store, I believe you could still achieve everything there is today:
get_fuel() -> Result<u64> set_fuel(u64) -> Result<()> fuel_async_yield_interval(u64) -> Result<()> fuel_async_yield_interval_clear() -> Result<()>Basically you can set and get the amount of fuel and then to enable async support and yielding we say every N fuel consumed we will yield. This also means you could get rid of the out of gas behavior APIs. Consolidating something like 7 (8 including this PR) methods into 4
Internally there is a current active fuel, and a reserve "tank" of fuel for restoring after a yield. The get and set operate on the combined amount of active fuel and fuel in the tank, then the current enum for out of gas behavior just becomes an optional amount of fuel to take from the tank after a yield. If the yield interval is cleared we just take all the fuel from the tank and add it to the active amount.
That feels way simpler to explain and implement. With a tighter API surface to boot. TBH i don't fully understand how fuel_adj works in the current implementation.
Wasmtime GitHub notifications bot (Oct 16 2023 at 15:25):alexcrichton commented on issue #7240:
I want to be sure to reiterate that this isn't something you're expected to take on @rockwotj, although if you're willing we won't stop you of course! Additionally breaking changes are ok to Wasmtime and happen relatively frequently. We only require major breaking changes to go through RFCs, although that hasn't happened in quite some time. This change to fuel would not constitue "major" and could be done through consensus of various stakeholders.
I'd have to set aside some time to think about this personally, so if you're interested, would you be up for filing an issue with your idea and we could discuss on there? If not that's totally ok and I can file an issue in the future as well.
Wasmtime GitHub notifications bot (Oct 16 2023 at 16:45):rockwotj commented on issue #7240:
Haha I get nerdsniped easily :smile:
I created an issue here with my strawman proposal: https://github.com/bytecodealliance/wasmtime/issues/7255
Last updated: Dec 23 2024 at 12:05 UTC