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 resource limiting to the Wasmtime API. #2736

Merged
merged 4 commits into from
Apr 19, 2021

Conversation

peterhuene
Copy link
Member

@peterhuene peterhuene commented Mar 17, 2021

This PR adds a ResourceLimiter trait to the Wasmtime API.

When used in conjunction with Store::new_with_limiter, this can be used to
monitor and prevent WebAssembly code from growing linear memories and tables.

This is particularly useful when hosts need to take into account host resource
usage to determine if WebAssembly code can consume more resources.

A simple Limiter is also included with these changes that will
simply limit the size of linear memories or tables for all instances created in
the store based on static values.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Mar 18, 2021
@peterhuene peterhuene force-pushed the limiter branch 2 times, most recently from c21c779 to 71e2956 Compare March 18, 2021 00:53
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@peterhuene

This comment has been minimized.

@peterhuene
Copy link
Member Author

peterhuene commented Mar 18, 2021

So it appears that 20.04 is now ubuntu-latest by design, so we were caught in an upgrade to GitHub actions.

@peterhuene peterhuene force-pushed the limiter branch 2 times, most recently from e956871 to de177f3 Compare March 18, 2021 20:44
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

We currently also have a few methods on Config for limiting resources, so I think it'd be good to have everything in one place and move those here as well? Otherwise this looks pretty neat to me and seems like a reasonable feature!

crates/runtime/src/instance.rs Outdated Show resolved Hide resolved
crates/runtime/src/instance/allocator.rs Outdated Show resolved Hide resolved
crates/runtime/src/memory.rs Show resolved Hide resolved
crates/wasmtime/src/limiter.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/limiter.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/limiter.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/trampoline.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:c-api Issues pertaining to the C API. labels Mar 23, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @fitzgen, @peterhuene

This issue or pull request has been labeled: "fuzzing", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: fuzzing
  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@peterhuene peterhuene marked this pull request as ready for review April 2, 2021 18:39
crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/limits.rs Outdated Show resolved Hide resolved
tests/all/limits.rs Show resolved Hide resolved
crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved
@peterhuene
Copy link
Member Author

Only the last commit is new; the rest were from your last review prior to my rebasing this branch on main.

@github-actions github-actions bot added the wasmtime:docs Issues related to Wasmtime's documentation label Apr 9, 2021
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks great to me! I've got one clarification on the poolilng allocator and deinitializing halfway through, but otherwise just some documentation nits.

crates/wasmtime/src/store.rs Show resolved Hide resolved
crates/wasmtime/src/store.rs Show resolved Hide resolved
crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/limits.rs Outdated Show resolved Hide resolved
This commit adds a `ResourceLimiter` trait to the Wasmtime API.

When used in conjunction with `Store::new_with_limiter`, this can be used to
monitor and prevent WebAssembly code from growing linear memories and tables.

This is particularly useful when hosts need to take into account host resource
usage to determine if WebAssembly code can consume more resources.

A simple `StaticResourceLimiter` is also included with these changes that will
simply limit the size of linear memories or tables for all instances created in
the store based on static values.
* Implemented `StoreLimits` and `StoreLimitsBuilder`.
* Moved `max_instances`, `max_memories`, `max_tables` out of `Config` and into
  `StoreLimits`.
* Moved storage of the limiter in the runtime into `Memory` and `Table`.
* Made `InstanceAllocationRequest` use a reference to the limiter.
* Updated docs.
* Made `ResourceLimiterProxy` generic to remove a level of indirection.
* Fixed the limiter not being used for `wasmtime::Memory` and
  `wasmtime::Table`.
* `Memory::new` now returns `Result<Self>` so that an error can be returned if
  the initial requested memory exceeds any limits placed on the store.

* Changed an `Arc` to `Rc` as the `Arc` wasn't necessary.

* Removed `Store` from the `ResourceLimiter` callbacks. Custom resource limiter
  implementations are free to capture any context they want, so no need to
  unnecessarily store a weak reference to `Store` from the proxy type.

* Fixed a bug in the pooling instance allocator where an instance would be
  leaked from the pool. Previously, this would only have happened if the OS was
  unable to make the necessary linear memory available for the instance. With
  these changes, however, the instance might not be created due to limits
  placed on the store. We now properly deallocate the instance on error.

* Added more tests, including one that covers the fix mentioned above.
* Add another memory to `test_pooling_allocator_initial_limits_exceeded` to
  ensure a partially created instance is successfully deallocated.
* Update some doc comments for better documentation of `Store` and
  `ResourceLimiter`.
@alexcrichton alexcrichton merged commit f12b4c4 into bytecodealliance:main Apr 19, 2021
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Apr 20, 2021
alexcrichton added a commit that referenced this pull request Apr 20, 2021
mchesser pushed a commit to mchesser/wasmtime that referenced this pull request May 24, 2021
* Add resource limiting to the Wasmtime API.

This commit adds a `ResourceLimiter` trait to the Wasmtime API.

When used in conjunction with `Store::new_with_limiter`, this can be used to
monitor and prevent WebAssembly code from growing linear memories and tables.

This is particularly useful when hosts need to take into account host resource
usage to determine if WebAssembly code can consume more resources.

A simple `StaticResourceLimiter` is also included with these changes that will
simply limit the size of linear memories or tables for all instances created in
the store based on static values.

* Code review feedback.

* Implemented `StoreLimits` and `StoreLimitsBuilder`.
* Moved `max_instances`, `max_memories`, `max_tables` out of `Config` and into
  `StoreLimits`.
* Moved storage of the limiter in the runtime into `Memory` and `Table`.
* Made `InstanceAllocationRequest` use a reference to the limiter.
* Updated docs.
* Made `ResourceLimiterProxy` generic to remove a level of indirection.
* Fixed the limiter not being used for `wasmtime::Memory` and
  `wasmtime::Table`.

* Code review feedback and bug fix.

* `Memory::new` now returns `Result<Self>` so that an error can be returned if
  the initial requested memory exceeds any limits placed on the store.

* Changed an `Arc` to `Rc` as the `Arc` wasn't necessary.

* Removed `Store` from the `ResourceLimiter` callbacks. Custom resource limiter
  implementations are free to capture any context they want, so no need to
  unnecessarily store a weak reference to `Store` from the proxy type.

* Fixed a bug in the pooling instance allocator where an instance would be
  leaked from the pool. Previously, this would only have happened if the OS was
  unable to make the necessary linear memory available for the instance. With
  these changes, however, the instance might not be created due to limits
  placed on the store. We now properly deallocate the instance on error.

* Added more tests, including one that covers the fix mentioned above.

* Code review feedback.

* Add another memory to `test_pooling_allocator_initial_limits_exceeded` to
  ensure a partially created instance is successfully deallocated.
* Update some doc comments for better documentation of `Store` and
  `ResourceLimiter`.
mchesser pushed a commit to mchesser/wasmtime that referenced this pull request May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:docs Issues related to Wasmtime's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants