Stream: git-wasmtime

Topic: wasmtime / PR #6445 Expose remaining fuel

Wasmtime GitHub notifications bot (May 24 2023 at 14:30):

LukasForst opened PR #6445 from LukasForst:main to bytecodealliance:main:

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

I don't think this was discussed anywhere yet, but it would be nice to have a method that returns remaining fuel. Right now, it is possible only by consuming 0 fuel like let remaining = consume_fuel(0)?, which is not really intuitive. That's why I implemented remaining_fuel.

The only problem I see here is that it's not super clear/intuitive why when you add_fuel(u64::max) then without executing anything remaining_fuel() == i64::max and not remaining_fuel() == u64::max.

Wasmtime GitHub notifications bot (May 24 2023 at 14:30):

LukasForst requested fitzgen for a review on PR #6445.

Wasmtime GitHub notifications bot (May 24 2023 at 14:30):

LukasForst requested wasmtime-core-reviewers for a review on PR #6445.

Wasmtime GitHub notifications bot (May 24 2023 at 14:32):

LukasForst updated PR #6445.

Wasmtime GitHub notifications bot (May 24 2023 at 18:48):

fitzgen submitted PR review:

Makes sense to me, and generally looks good, but every time I've looked into the details of fuel accounting I've been a bit confused about why things are the ways they are, so I'm not super confident in my review of this, so I'm going to also flag Alex.

Wasmtime GitHub notifications bot (May 24 2023 at 18:51):

fitzgen submitted PR review:

Oh one other thing: can there be some smoke tests for

• Adding fuel to a new store, and then getting the fuel remaining and assert it is equal to the amount added
• Getting fuel from a new store without fuel returns zero fuel
• Adding fuel to the store, running a small amount of wasm, getting the remaining fuel and asserting it is nonzero
• Adding a small amount of fuel to the store, running a wasm inf loop till it traps, asserting that remaining fuel is zero

Thanks!

Wasmtime GitHub notifications bot (May 24 2023 at 18:51):

fitzgen requested alexcrichton for a review on PR #6445.

Wasmtime GitHub notifications bot (May 24 2023 at 19:50):

alexcrichton submitted PR review:

Seems reasonable to me, thanks! I agree with Nick that tests would be good, too.

Otherwise though I also agree that the fuel-related methods on Store are a bit haphazard in their design but I don't know of a better way to organize this myself, so adding this seems fine.

One thing I might suggest though is to implement this in terms of other methods to avoid having multiple locations interpret the raw state.

Wasmtime GitHub notifications bot (May 25 2023 at 07:54):

LukasForst updated PR #6445.

Wasmtime GitHub notifications bot (May 25 2023 at 15:14):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 25 2023 at 16:00):

alexcrichton merged PR #6445.

Last updated: Dec 23 2024 at 12:05 UTC