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

Adapt to wasmi::ResourceLimiter, replacing mem_fuel metering #946

Closed
wants to merge 4 commits into from

Conversation

jayz22
Copy link
Contributor

@jayz22 jayz22 commented Jul 13, 2023

What

This PR adapts to the new wasmi::ResourceLimiter api, replacing our current metering-based guest memory limiting mechanism. The main benefits are:

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

@jayz22 jayz22 changed the title [WIP] Use ResourceLimiter [WIP] Adapt to wasmi::ResourceLimiter, replacing memory fuel metering Jul 13, 2023
@jayz22 jayz22 marked this pull request as ready for review July 13, 2023 21:51
@jayz22 jayz22 requested review from graydon, sisuresh and a team as code owners July 13, 2023 21:51
@@ -33,6 +36,9 @@ use wasmi::{Caller, StoreContextMut};

impl wasmi::core::HostError for HostError {}

pub const WASM_LINEAR_MEMORY_SIZE_LIMIT_BYTES: usize = 0x200000; // 2MB
pub const WASM_TABLE_ELEMENTS_LIMIT_COUNT: u32 = 1000;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added two new limits here. I think the linear memory size 2MB should be reasonable? Not sure about the table elements count. Would like some feedback.

@jayz22 jayz22 changed the title [WIP] Adapt to wasmi::ResourceLimiter, replacing memory fuel metering Adapt to wasmi::ResourceLimiter, replacing mem_fuel metering Jul 13, 2023
Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I suspect this all works, I think I might have miscommunicated the approach I had in mind. I was hoping (and please correct me if this is not possible) to make Host impl ResourceLimiter directly, such that the callbacks would deduct memory usage from the Budget directly. I.e. I was thinking we would not be using the StoreLimits type from wasmi -- that is a convenience type that implements ResourceLimiter but it is not the only impl possible and, in our case, not the one I was expecting us to use.

(Part of the reason I want to do it this way is that I would like the total memory budget to be available "in any combination" to all VMs that get instantiated, such that 1 VM can use the entire 100MB of the budget, if it wants, or the 100MB can be split across multiple VMs as they call one another).

@jayz22
Copy link
Contributor Author

jayz22 commented Jul 14, 2023

Closing in favor of #950

@jayz22 jayz22 closed this Jul 14, 2023
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

Successfully merging this pull request may close these issues.

[test] Ensure data segment and user-defined memories can't exceed memory budget limit
2 participants