Stream: git-wasmtime

Topic: wasmtime / PR #3393 Add ResourceLimiterAsync, which can y...

Wasmtime GitHub notifications bot (Oct 21 2021 at 23:43):

pchickey edited PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 21 2021 at 23:46):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 21 2021 at 23:52):

pchickey edited PR #3393 from pch/async_limiting to main:

This PR:

• Moves ResourceLimiter's definition from wasmtime_runtime to wasmtime
• Adds an additional ResourceLimiterAsync, which is just like a ResourceLimiter except the memory_growing and table_growing methods are async (via async_trait). This is so that async embeddings of wasmtime to yield execution while they await for a resource to be available.
• In order for the ResourceLimiterAsync methods to await inside memory & table grows, we need to add async variants of Memory::new, Memory::grow, Table::new, and Table::grow. These variants are not required for all async Stores, only for stores which are given a Store::limiter_async. So, this won't break any existing code.

I had to restructure a fair bit of the interface between runtime and wasmtime for this PR, including extending the wasmtime_runtime::Store trait, and changing the error type passed between them to anyhow::Error instead of Box<dyn std::error::Error + Send + Sync>.

Additionally, both ResourceLimiters are now safer: wasmtime_runtime now catches any panics that occur from the user's impl of the traits.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 21 2021 at 23:52):

pchickey has marked PR #3393 as ready for review.

Wasmtime GitHub notifications bot (Oct 21 2021 at 23:55):

pchickey edited PR #3393 from pch/async_limiting to main:

This PR:

• Moves ResourceLimiter's definition from wasmtime_runtime to wasmtime
• Adds an additional ResourceLimiterAsync, which is just like a ResourceLimiter except the memory_growing and table_growing methods are async (via async_trait). This is so that async embeddings of wasmtime to yield execution while they await for a resource to be available.
• In order for the ResourceLimiterAsync methods to await inside memory & table grows, we need to add async variants of Memory::new, Memory::grow, Table::new, and Table::grow. These variants are not required for all async Stores, only for stores which are given a Store::limiter_async. So, this won't break any existing code.
• Both ResourceLimiter traits now get table_grow_failed, which corresponds to memory_grow_failed. Better to just get this over with now than have to add it in later.
• Test suite covers table limiting and async and panics now.

I had to restructure a fair bit of the interface between runtime and wasmtime for this PR, including extending the wasmtime_runtime::Store trait, and changing the error type passed between them to anyhow::Error instead of Box<dyn std::error::Error + Send + Sync>.

Additionally, both ResourceLimiters are now safer: wasmtime_runtime now catches any panics that occur from the user's impl of the traits.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 21 2021 at 23:56):

pchickey edited PR #3393 from pch/async_limiting to main:

This PR:

• Moves ResourceLimiter's definition from wasmtime_runtime to wasmtime
• Adds an additional ResourceLimiterAsync, which is just like a ResourceLimiter except the memory_growing and table_growing methods are async (via #[async_trait]). This is so that async embeddings of wasmtime to yield execution while they await for a resource to be available.
• In order for the ResourceLimiterAsync methods to await inside memory & table grows, we need to add async variants of Memory::new, Memory::grow, Table::new, and Table::grow. These variants are not required for all async Stores, only for stores which are given a Store::limiter_async. So, this won't break any existing code.
• Both ResourceLimiter traits now get table_grow_failed, which corresponds to memory_grow_failed. Better to just get this over with now than have to add it in later.
• Test suite covers table limiting and async and panics now.

I had to restructure a fair bit of the interface between runtime and wasmtime for this PR, including extending the wasmtime_runtime::Store trait, and changing the error type passed between them to anyhow::Error instead of Box<dyn std::error::Error + Send + Sync>.

Additionally, both ResourceLimiters are now safer: wasmtime_runtime now catches any panics that occur from the user's impl of the traits.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 21 2021 at 23:57):

pchickey edited PR #3393 from pch/async_limiting to main:

This PR:

• Moves ResourceLimiter's definition from wasmtime_runtime to wasmtime
• Adds an additional ResourceLimiterAsync, which is just like a ResourceLimiter except the memory_growing and table_growing methods are async (via #[async_trait]). This is so that async embeddings of wasmtime can handle memory or table growing by awaiting for a resource to be available.
• In order for the ResourceLimiterAsync methods to await inside memory & table grows, we need to add async variants of Memory::new, Memory::grow, Table::new, and Table::grow. These variants are not required for all async Stores, only for stores which are given a Store::limiter_async. So, this won't break any existing code.
• Both ResourceLimiter traits now get table_grow_failed, which corresponds to memory_grow_failed. Better to just get this over with now than have to add it in later.
• Test suite covers table limiting and async and panics now.

I had to restructure a fair bit of the interface between runtime and wasmtime for this PR, including extending the wasmtime_runtime::Store trait, and changing the error type passed between them to anyhow::Error instead of Box<dyn std::error::Error + Send + Sync>.

Additionally, both ResourceLimiters are now safer: wasmtime_runtime now catches any panics that occur from the user's impl of the traits.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 22 2021 at 14:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2021 at 14:52):

alexcrichton created PR review comment:

I'd be a little uneasy actually using setjmp to sail past a catch_unwind frame, so could this be transmitted through the return value and outside of this closure the raise_user_trap happens?

Wasmtime GitHub notifications bot (Oct 22 2021 at 14:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2021 at 14:52):

alexcrichton created PR review comment:

I think the indentation here may be a bit off?

Wasmtime GitHub notifications bot (Oct 22 2021 at 14:52):

alexcrichton created PR review comment:

Could this and the panic test below be updated to grow through wasm? I think that's the more interesting path to test b/c otherwise there's already only Rust code on the stack which didn't necessitate adding a catch_unwind or anything like that.

Wasmtime GitHub notifications bot (Oct 22 2021 at 15:56):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 22 2021 at 17:42):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2021 at 17:42):

pchickey created PR review comment:

I didn't think of that, thanks! Easy fix

Wasmtime GitHub notifications bot (Oct 22 2021 at 17:43):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2021 at 17:43):

pchickey created PR review comment:

Ah yeah rustfmt gave up because its in a macro.

Wasmtime GitHub notifications bot (Oct 22 2021 at 17:43):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 22 2021 at 18:08):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 22 2021 at 18:08):

pchickey requested alexcrichton for a review on PR #3393.

Wasmtime GitHub notifications bot (Oct 22 2021 at 18:45):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 22 2021 at 19:01):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 22 2021 at 20:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 22 2021 at 21:03):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 22 2021 at 23:24):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 25 2021 at 17:04):

pchickey updated PR #3393 from pch/async_limiting to main.

Wasmtime GitHub notifications bot (Oct 25 2021 at 18:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 25 2021 at 18:47):

pchickey merged PR #3393.

Last updated: Nov 22 2024 at 16:03 UTC