Allow async yield from epoch interruption callback by jameysharp · Pull Request #6464 · bytecodealliance/wasmtime

jameysharp · 2023-05-26T01:15:38Z

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.

jameysharp · 2023-05-26T01:17:56Z

My guess is if anybody has objections to this PR it'll probably be @alexcrichton or @fitzgen so maybe I should have picked one of you as reviewers instead of letting it auto-assign. Oh well.

github-actions · 2023-05-26T03:44:53Z

Subscribe to Label Action

cc @peterhuene

Details

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.

alexcrichton

Mind adding some tests as well?

alexcrichton · 2023-05-26T14:35:49Z

    pub fn epoch_deadline_callback(
        &mut self,
-        callback: impl FnMut(StoreContextMut<T>) -> Result<u64> + Send + Sync + 'static,
+        callback: impl FnMut(StoreContextMut<T>) -> DeadlineResult + Send + Sync + 'static,


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

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

Yeah that's what I'm thinking

alexcrichton · 2023-05-26T14:36:50Z

+            None => Err(Trap::Interrupt.into()),
+            Some(callback) => {
+                let delta = match callback((&mut *self).as_context_mut()) {
+                    // Note: If the callback returns an error, we don't call it again.


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.

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.

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

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.

alexcrichton · 2023-05-26T14:38:20Z

@@ -943,7 +941,7 @@ impl<T> Store<T> {
    /// for an introduction to epoch-based interruption.
    pub fn epoch_deadline_callback(


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

alexcrichton · 2023-05-26T14:39:17Z

+                    DeadlineResult::Yield(delta) => {
+                        // Do the async yield. May return a trap if future was
+                        // canceled while we're yielded.
+                        self.async_yield_impl()?;
+                        delta
+                    }


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.

fitzgen · 2023-05-30T17:23:11Z

 }

+/// What to do after returning from a deadline callback.
+pub enum DeadlineResult {


Maybe

Suggested change

pub enum DeadlineResult {

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.

jameysharp · 2023-06-02T19:07:14Z

I think I've addressed all your review comments.

I've also added a brief note to the release notes, though I'm not sure if it needs to be louder since it's an API break?

I'm hoping to get this into the next release, and I just realized today that's forking off on Monday. Sorry for not leaving a lot of time for review.

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.

github-actions · 2023-06-02T19:45:03Z

Subscribe to Label Action

cc @peterhuene

Details

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

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

peterhuene: wasmtime:c-api

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

Learn more.

…6464) 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.

jameysharp requested a review from a team as a code owner May 26, 2023 01:15

jameysharp requested review from pchickey and removed request for a team May 26, 2023 01:15

github-actions Bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label May 26, 2023

alexcrichton approved these changes May 26, 2023

View reviewed changes

alexcrichton requested review from alexcrichton and removed request for pchickey May 26, 2023 14:39

fitzgen reviewed May 30, 2023

View reviewed changes

jameysharp force-pushed the epoch-callback-yield branch from e3c4a2f to fc75f91 Compare June 2, 2023 19:04

jameysharp requested a review from a team as a code owner June 2, 2023 19:04

jameysharp force-pushed the epoch-callback-yield branch from fc75f91 to 00b1c74 Compare June 2, 2023 19:09

alexcrichton approved these changes Jun 2, 2023

View reviewed changes

alexcrichton enabled auto-merge June 2, 2023 19:31

alexcrichton added this pull request to the merge queue Jun 2, 2023

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 2, 2023

Fix Wasmtime C API implementation

beef39b

jameysharp enabled auto-merge June 2, 2023 19:43

github-actions Bot added wasmtime:c-api Issues pertaining to the C API. wasmtime:docs Issues related to Wasmtime's documentation labels Jun 2, 2023

jameysharp added this pull request to the merge queue Jun 2, 2023

Merged via the queue into bytecodealliance:main with commit 1d4686d Jun 2, 2023

jameysharp deleted the epoch-callback-yield branch June 2, 2023 21:04

		@@ -943,7 +941,7 @@ impl<T> Store<T> {
		/// for an introduction to epoch-based interruption.
		pub fn epoch_deadline_callback(

Conversation

jameysharp commented May 26, 2023

Uh oh!

jameysharp commented May 26, 2023

Uh oh!

github-actions Bot commented May 26, 2023

Subscribe to Label Action

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameysharp commented Jun 2, 2023

Uh oh!

Uh oh!

github-actions Bot commented Jun 2, 2023

Subscribe to Label Action

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants