Stream: git-wasmtime

Topic: wasmtime / PR #6464 Allow async yield from epoch interrup...

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 01:15):

jameysharp opened PR #6464 from jameysharp:epoch-callback-yield to bytecodealliance:main:

When an epoch interruption deadline arrives, previously it was possible to yield to the async executor, or to invoke a callback on the wasm stack, but not both. This changes the API to allow callbacks to run and then request yielding to the async executor.

We discussed this in today's Wasmtime bi-weekly call and I believe this PR reflects the consensus from that discussion. But I'm open to bikeshedding the API.

I was tempted to revise the out-of-gas/fuel API to match the epoch interruption API as well, but decided not to do that in this PR.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 01:15):

jameysharp requested wasmtime-core-reviewers for a review on PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 01:15):

jameysharp requested pchickey for a review on PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 14:39):

alexcrichton submitted PR review:

Mind adding some tests as well?

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 14:39):

alexcrichton submitted PR review:

Mind adding some tests as well?

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 14:39):

alexcrichton created PR review comment:

From an ergonomic perspective could this remain Result<DeadlineResult>? (maybe with a bit of renaming)

That'll allow ? to be used for fallible operations and matches how traps are handled elsewhere in the embedding API where it's the Err of a Result

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 14:39):

alexcrichton created PR review comment:

Can the documentation for this method be updated to talk about how returning Yield will panic if async_support is disabled?

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 14:39):

alexcrichton created PR review comment:

Is there any particular reason as to why? If something traps I could see it still being possible resuming other execution in the Store which may be unrelated which could require the same callback.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 14:39):

alexcrichton created PR review comment:

In this case I believe that async_yield_impl will panic internally if it's not configured right but technically that method's panic should be allowable to be a debug_assert! or an unwrap_unchecked or something like that. I think it'd be good to have a dedicated assert here for the use case that this covers where Yield is returned but async isn't enabled or configured.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 14:39):

alexcrichton requested alexcrichton for a review on PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2023 at 16:19):

jameysharp created PR review comment:

As far as I could tell this was the existing behavior, but it seemed non-obvious so I added a comment. I don't think there's a strong argument for behaving this way and I agree that it could be useful to preserve the callback.

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2023 at 17:23):

fitzgen created PR review comment:

Maybe

pub enum DeadlineBehavior {

to make Result<DeadlineBehavior>?

Another option is to have this return a std::ops::ControlFlow where the break is an anyhow::Error and the continue is either a sync or async enum with a u64 payload. This would also allow for ? usage. But my experience with ControlFlow has not been super great.

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2023 at 17:24):

fitzgen created PR review comment:

To be clear, this would remove the DeadlineResult::Trap variant right? Since that would become the Result::Err?

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2023 at 17:24):

fitzgen created PR review comment:

Once something in a store traps, you really aren't supposed to use the store again.

view this post on Zulip Wasmtime GitHub notifications bot (May 31 2023 at 14:47):

alexcrichton created PR review comment:

Yeah that's what I'm thinking

view this post on Zulip Wasmtime GitHub notifications bot (May 31 2023 at 14:49):

alexcrichton created PR review comment:

Ah ok if this is preserving the existing behavior that's ok, but otherwise while you're here I think it might be good to "fix" this and put the callback back in place regardless of the result.

I agree that most of the time stores/instances/etc should be considered "poisoned" after trapping but that feels like a higher level concern which wouldn't affect a lower-level detail like this. Mostly in that we don't have anything official per-se about nullifying a store when it traps.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2023 at 19:04):

jameysharp updated PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2023 at 19:04):

jameysharp requested wasmtime-default-reviewers for a review on PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2023 at 19:09):

jameysharp updated PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2023 at 19:31):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2023 at 19:31):

alexcrichton has enabled auto merge for PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2023 at 19:42):

jameysharp updated PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2023 at 19:43):

jameysharp has enabled auto merge for PR #6464.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2023 at 21:04):

jameysharp merged PR #6464.

Last updated: Nov 22 2024 at 16:03 UTC