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

Consider forking into Wasmi::Module and add section-wise metering #838

Closed
jayz22 opened this issue Jun 7, 2023 · 5 comments
Closed

Consider forking into Wasmi::Module and add section-wise metering #838

jayz22 opened this issue Jun 7, 2023 · 5 comments

Comments

@jayz22
Copy link
Contributor

jayz22 commented Jun 7, 2023

Follow up to #821

The VmInstantiation's cpu cost is a linear function w.r.t. the size of the Wasm bytes. Calibration is done on the Wasm file containing maximum number of internal functions, in order to account for the worst case scenario (i.e. preventing hostile contracts). This means a good contract may be over charged by up to ~4x contract size.

One way to solve this is to do section-wise metering. I.e. breaking down the metering of Module::new (where parsing happens) into sections and have a separate cost component for each. Here is the natural place to do it https://github.com/stellar/wasmi/blob/soroban-wasmi-v0.30/crates/wasmi/src/module/parser.rs#L169-L185
This would require forking into Wasmi and inject into metering charging, which is something we've moved away from since #683.

Note this is only to make the metering model more "fair" for normal contracts, it does not get around the fact that Vm instantiation is inherently costly and inefficient. #827 tracks ways to make it more efficient, which is the much preferred approach.

@jayz22 jayz22 changed the title Metering: consider decompose Wasmi::Module instantiation into sections Consider forking into Wasmi::Module and add section-wise metering Jun 7, 2023
@anupsdf
Copy link
Contributor

anupsdf commented Jun 7, 2023

Good find on section-wise metering! While we look into caching option in #827 , should we also file an issue for ParityTech to incorporate section-wise metering? That way at some point we will have both fair (section-wise) and efficient (caching etc) metering.

@MonsieurNicolas
Copy link
Contributor

We should not be treating wasmi upstream like it's immutable.
I think modifying its instantiation code to invoke some callbacks ("Observer" pattern) should be fairly easy to upstream?
We would do actual metering from those callbacks as long as we have the right arguments (this allows people to perform additional policies outside of our use cases).

@jayz22
Copy link
Contributor Author

jayz22 commented Jun 7, 2023

should we also file an issue for ParityTech to incorporate section-wise metering? That way at some point we will have both fair (section-wise) and efficient (caching etc) metering.

@anupsdf Just want to point out this has nothing to do with fuel metering (i.e. metering of the wasm execution). This is purely for the host and working under our metering framework. The way to do it is to add some callback into Wasmi that invokes our metering charging functions in the host (as @MonsieurNicolas pointed out).

It might be a little tricky to frame this as a general "Observer" callback to upstream though, since it will probably require some new constrains (trait impls) on the host (Wasmi is very particular about its own way of things). But I'll look into it.

@graydon
Copy link
Contributor

graydon commented Jun 7, 2023

This is an interesting possibility to explore, but not feature-work, and should be postponed until features are wrapped up.

@graydon
Copy link
Contributor

graydon commented Dec 16, 2024

We have done something more-or-less like this in CAP 54

@graydon graydon closed this as completed Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants
@graydon @MonsieurNicolas @jayz22 @anupsdf and others