Skip to content
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

Add metering as an option #39

Closed
Southporter opened this issue May 15, 2024 · 5 comments
Closed

Add metering as an option #39

Southporter opened this issue May 15, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@Southporter
Copy link
Contributor

I've been poking around bytebox and this is a solid project. One thing that I would like to add is metering. This is something that wasmtime and wasmer (and probably others) have implemented.

Basic idea is this: you configure your module instance to have a certain amount of "fuel". Then a function is called and the vm either runs to completion, traps, or if the fuel meter reaches zero it traps as OutOfFuel or something similar.

I'd be happy to implement something and contribute it, but I wanted to talk through what type of metering before starting.

The most straight forward would be to have each instruction consume a certain amount of fuel units. Simple ones consume 1 or 2, more complex like call_indirect or select_t might consume more.

It looks like wasmtime just has 1 fuel unit for everything but branches, nop, and drop. https://github.com/bytecodealliance/wasmtime/blob/c477424f45871563be02eba14815ba3446158441/crates/cranelift/src/func_environ.rs#L328-L344.

And wasmer allows you to pass in a custom meter function. https://github.com/wasmerio/wasmer/blob/9573519e225b3ea6bd371c832fd352e72314074e/lib/c-api/src/wasm_c_api/unstable/middlewares/metering.rs#L180

There are perks to both, but I wasn't sure what your design philosophy is more geared toward. Let me know what you think.

@rdunnington rdunnington added the enhancement New feature or request label May 15, 2024
@rdunnington
Copy link
Owner

Hi,

Yeah, this seems reasonable to add.

Do you think having fuel overrides for certain instructions would be a much desired use case? I'm inclined to leverage the debugPreamble() that's already instrumented in each opcode and just have a comptime defined fuel lookup switch similar to wasmtime. This way there doesn't have to be an extra indirection for a callback.

Is fuel typically configured globally and retains state across invokes, or is it per-invoke? I suppose either would be easy to add with the same mechanism - invoke() could just reset the fuel at the beginning if it's per-invoke. The amount of fuel could be configured via ModuleInstantiateOpts or InvokeOpts.

Are functions typically resumed once a module or invoke runs out of fuel? There's already some (untested) support for resuming a debug-trapped function ala resumeInvoke(). If needed, maybe the code that returns TrapError.TrapOutOfFuel can setup some resuming state in a similar way and it could reuse the same code path for both debug and fuel cases.

@Southporter
Copy link
Contributor Author

I don't think having overrides will be a common usecase. Wasmtime is widely known and it doesn't allow it, so it is probably best to do a lookup switch.

As far as configuration goes, wasmtime only allows it configured on the module instance, whereas wasmer lets you do it at the instance level or the function level. So for module level, you update the fuel on the store and then run the function via a normal invoke. I shouldn't be hard to add it to both ModuleInstantiateOpts and InvokeOpts to allow for either call.

As far as resumption goes, I know Wasmtime doesn't allow it. It traps, then you have to invoke again from the start. It looks like wasmer doesn't allow resumption either.
I would really like Bytebox to allow resumeInvoke() to pick up with another round of fuel/after fuel has been updated.

One thing though: should this be behind a compile flag so that those that don't care about metering can remove those from the code? May not be too big a deal since Zig does a good job of not including code that isn't used from the root, but might be helpful.

@rdunnington
Copy link
Owner

Sounds like a plan. The compile-time flag is a good idea - ideally it would default to off, and maybe there can be an additional stage in ci.yml that builds and runs a smoke test with it on.

Like I mentioned, feel free to take a stab at getting resuming working. If you have any trouble, let me know and I can take a look too.

Regarding where fuel is stored, I think it can just go into the ModuleInstance directly, it feels like the right place for a bunch of runtime state that isn't part of the spec.

When you make the change to debugPreamble() to update remaining fuel, could you rename it to just preamble()? Once fuel is added, it won't just be for debug purposes anymore.

@Southporter
Copy link
Contributor Author

Great, sounds good. I'll start working on that.

@rdunnington
Copy link
Owner

This was completed in #49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants