-
Notifications
You must be signed in to change notification settings - Fork 41
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
Switch to wasmi native fuel metering #815
Conversation
All tests passing against my wasmi branch. |
Add wasmi memory fuel metering; all tests pass
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.
Fairly good progress! A bit of refactoring in VM would be nice, and removal of asserts, but I think it's going in the right direction.
The division-losing-fuel problem is the bit that kinda seems bad to me. I'm not sure it's bad, but it seems like it might be. Perhaps we can discuss tomorrow?
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.
Fairly good progress! A bit of refactoring in VM would be nice, and removal of asserts, but I think it's going in the right direction.
The division-losing-fuel problem is the bit that kinda seems bad to me. I'm not sure it's bad, but it seems like it might be. Perhaps we can discuss tomorrow?
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.
Great, thanks!
What
Pinned to my local wasmi branch: https://github.com/jayz22/wasmi/tree/soroban-fuel-metering
This PR adapts to wasmi upstream changes between v0.16 and v0.30. In particular the fuel metering features (resolves #683).
Changes include:
wasmi::core::FromValue
no longer exist), added a new traitWasmiMarshal
.invoke_function
on the VMdispatch
Why
[TODO: Why this change is being made. Include any context required to understand the why.]
Known limitations
We did not successfully get rid of the fork because of Wasmi does not have:
Fuel
, andFuelCosts
ResourceLimiter
which will allow host limiting and charging the maximum amount of memory upfront. We will switch to it when the feature is ready (AddResourceLimiter
towasmi::Store
wasmi-labs/wasmi#728).Still need to recalibrate the fuel costs.