Stream: git-wasmtime

Topic: wasmtime / PR #3393 make it possible for ResourceLimiter ...

Wasmtime GitHub notifications bot (Sep 29 2021 at 01:18):

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

<!--

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 (Sep 29 2021 at 14:21):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

I've commented more about this below but I'd ideally prefer to not have this since if you're asking for the store and someone forgot to configure it that's a bug rather than something we'll gracefully want to handle.

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

I think it's probably ok to skip the limiter_ prefix on these methods (micro-nit)

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

This'll either need a different signature (taking &mut self) or need to be unsafe because otherwise I don't think it's safe in Rust to go from &self to &mut T

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

To prevent all consumers fo this trait from using async_trait I think it might be good to change this to have a default implementation of calling the memory_growing method?

I think it's a bit of a bummer to have both methods because if you're async you only want one and if you're not async you only want the other, but I don't know otherwise of a great way to manage this.

This does raise the question though of what happens when Memory::new (or Memory::grow) is called when async is configured? Even some async embeddings don't necessarily want async memory growth so enabling async doesn't guarantee that this method is configured. Ideally it's an embedder bug if Memory::new is called when async memory growth is desired though.

I think this might mean that we'll want a method here (perhaps default false?) which indicates which growth method to use (sync-or-async) and we dispatch with that on memory growth?

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

In addition to this change in this file I think we'll also want to add Memory::new_async and Memory::grow_async both as async methods. That may actually be pretty tricky though because currently the async implementation of block_on requires stack-switching and assumes we're on a wasm stack.

Otherwise adding these methods though would require open-coding a bit the limit logic where we could directly .await the future in these functions returned by the limiter. That may be the best route here though? (should ideally only be a small amount of duplication in the runtime implementation to have a method and an async method)

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

Similar to Memory I think we'll want this to be &mut dyn Store instead of an Option

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

With taking a store here instead of a limiter I think it's actually a bug in Wasmtime if we ever create a table/memory without a store because that means if there were a registered limiter we'd be skipping that. I think we probably want to change this constructor to take &mut dyn Store instead of Option<_> to reflect that and we can fix it if it ever trips a panic (I don't believe it will checking Wasmtime's current usage).

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

Does there need to be an async counterpart for this as well as memory_grow_failed? I figured the use case was "blocking" until there's enough memory to allow, but I'm not sure what sort of blocking work you'd want to do on growth failure.

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

That being said I'm also wary of this because anything that looks at the store should require that the store is set, so I'd ideally prefer to not have this method and if anyone needs to observe the store then they're on the path that already has the store set and it's safe to assert it's there.

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

I think we'll need to handle this because in this case a trap means that the future was dropped on the outside, so we need to propagate this trap to wasm to make wasm "abort" gracefully as well. Basically once the future is dropped we just need to clean it up quickly so the thinking is to propagate the error all the way back to the start.

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

I think changing this to not-an-option will also propagate out relatively far-ish. That's ok though because almost all instance allocation requests should have a store configured, only the initial blank module as well as host functions should skip their stores during construction, and the Option-handling is basically just here to make sure those two cases don't panic/segfault.

Wasmtime GitHub notifications bot (Sep 29 2021 at 14:21):

alexcrichton created PR review comment:

Ah yeah to expand on some thoughts from above, I don't think we'll want this conditional on the async cargo feature since that's on-by-default but rather some runtime configuration in the limiter itself which indicates which version to use.

Wasmtime GitHub notifications bot (Sep 29 2021 at 16:45):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2021 at 16:45):

pchickey created PR review comment:

Good point, I can probably leave this out. was keeping it in for completeness but I don't have a compelling use case

Wasmtime GitHub notifications bot (Sep 29 2021 at 16:47):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2021 at 16:47):

pchickey created PR review comment:

So, I tried having a default implementation, but it turns out that with async_trait those arent object-safe and rustc rejects them.

The other possibility is to have two public traits, ResourceLimiter and ResourceLimiterAsync, and an internal enum RLimiter { Sync(&mut dyn ResourceLimiter), Async(&mut dyn ResourceLimiterAsync) } and StoreInner::limiter has type Option<Box<dyn FnMut(&mut T) -> RLimiter>> and some chrome on the config methods to wrap the closure's return value in the Sync or Async variant.

Wasmtime GitHub notifications bot (Sep 29 2021 at 16:49):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2021 at 16:49):

pchickey created PR review comment:

Yep my initial constructor took just a &mut dyn Store and the option fell out of some complications elsewhere, and I hadn't thought that all the way through to indicate a bug in Wasmtime. I'll have to chase that out.

Wasmtime GitHub notifications bot (Sep 29 2021 at 16:52):

pchickey created PR review comment:

Good point, I meant to check this assumption by you. I forgot about the whole future dropped thing.

Wasmtime GitHub notifications bot (Sep 29 2021 at 16:52):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2021 at 20:08):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2021 at 20:08):

alexcrichton created PR review comment:

Ah ok then yeah I like your idea if we can't have defaulted async fn in traits, that seems reasonable to me and also solves the issue of async-runtime-still-uses-sync-limiter.

Wasmtime GitHub notifications bot (Oct 20 2021 at 00:36):

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

Wasmtime GitHub notifications bot (Oct 20 2021 at 00:58):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2021 at 00:58):

pchickey created PR review comment:

This is done now!

Wasmtime GitHub notifications bot (Oct 20 2021 at 21:39):

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

Wasmtime GitHub notifications bot (Oct 20 2021 at 23:53):

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

Wasmtime GitHub notifications bot (Oct 21 2021 at 01:34):

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

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

With this new signature it may also be worth mentioning in the safety documentation that this is only safe if store: &mut dyn Store is not used to get access to the Memory that lives within the store. Technically you could safely, from store, reach self via an instance and get two mutable references. So long as the trait implementation of Store doesn't do that though this should be safe.

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

Ah this is a bummer, I tried to avoid this because if the instance didn't have any start function it would avoid the need for a fiber which can be somewhat expensive. I see though how this is required for memories/tables and limiting their growth. I think let's leave this as you have it here and if it's an issue in the future we can try to optimize the further if truly necessary (and it may not be at all necessary)

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

I think this can probably be removed now?

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

Yeah I think that this will want to be unsafe b/c there's no guarantee that this StorePtr wasn't used outside the original lifetime of the borrow.

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

This is one case where this can probably come up as a user error but we're not prepared to catch this panic and re-raise it across the boundary when we get back to wasm. Arguably we probably should catch_panic an intrinsic like this though because we're calling arbitrary user code limiting memory growth now that I think about it though.

Would you be up for adding a catch_panic around this block (and the table version) which reraises the panic? (similar treatment for table_grow as well, and to be clear this isn't something new about this PR it's just something I'm realizing here. That wouldmake this .expect safe and we could leave it as-is.

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

Ah yeah the meaning of this is that the future representing the execution of wasm was dropped by the host so the coroutine has been resumed and should "finish ASAP", so the trap here means "please stop executing code on the coroutine as fast as you can". To do that I think it'd be best to propagate the trap to wasm here, which should skip execution of all remaining wasm. I think that can be done by returning Result<bool, Trap> from this Store funtion most likely.

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

stray newline removal?

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

Could this (and other grow/new methods affected) have a # Panics section which indicates the scenario where a non-async store is used will cause a panic? Also it might be good to link to the store method which configures the async limiter to indicate that it's how this is intended to be used.

Wasmtime GitHub notifications bot (Oct 21 2021 at 14:29):

alexcrichton created PR review comment:

One thing I'll say though is that now that the inner loop of run_async and run are the same we could probably pull those into a shared method so they do precisely the same thing.

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

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

Wasmtime GitHub notifications bot (Oct 21 2021 at 19:10):

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

Wasmtime GitHub notifications bot (Oct 21 2021 at 19:15):

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

Wasmtime GitHub notifications bot (Oct 21 2021 at 21:40):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2021 at 21:40):

alexcrichton created PR review comment:

Also, could you add a comment that the catch_unwind is specifically necessary here because we could run arbitrary user-provided code on growth?

Wasmtime GitHub notifications bot (Oct 21 2021 at 21:40):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 21 2021 at 21:40):

alexcrichton created PR review comment:

Could this be structured with a match to avoid needing the .unwrap()?

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

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

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

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

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

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

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

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

Last updated: Dec 23 2024 at 12:05 UTC