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

Provide parent pointers in ContinuationObjects, instead of FiberStacks #7

Merged
merged 13 commits into from
Sep 22, 2023

Conversation

frank-emrich
Copy link

This PR:

  1. Adds a parent field to ContinuationObject, pointing to the parent ContinuationObject or NULL.
  2. Removes the pointers stored near the top of a FiberStack to the parent FiberStack. As a result, the assembly in the fibre crate for dealing with stack switching is now reverted to be identical to the one from the fiber create.
  3. The "typed continuation store" field in the VMContext is now consistently used to point to the currently running continuation object, or NULL if not running inside a continuation (i.e., at the toplevel).

These changes also lead to some refactoring of the code for Operator::Resume in code_translator.rs.

@dhil
Copy link
Member

dhil commented Sep 18, 2023

Please rebase again, then I will take a look :-) thanks!

Copy link
Member

@dhil dhil left a comment

Choose a reason for hiding this comment

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

LGTM

cranelift/wasm/src/code_translator.rs Outdated Show resolved Hide resolved
Comment on lines +2683 to +2680
// TODO(frank-emrich): Check if parent is null. If so, we are at
// the toplevel and simply have no handler for the given tag and must trap.
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I suppose the pseudocode is something like

if parent is null then trap "unhandled tag"
else suspend $tag [args...]

Copy link
Author

Choose a reason for hiding this comment

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

Let's address this once we overhaul error handling in general.

let running = unsafe {
running
.as_ref()
.expect("Calling suspend outside of a continuation")
Copy link
Member

Choose a reason for hiding this comment

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

I guess this error should be the same as if forwarding fails, e.g ."unhandled tag". I don't recall the exact message, but it is in the reference interpreter.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can store the suspend arguments directly on the parent continuationobject instead of mangling with the VMContext now.

Copy link
Author

Choose a reason for hiding this comment

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

We should generally improve error handling, in particular trapping with an appropriate error code.

Regarding storing suspend arguments in continuation objects: You are right, the fact that we now have easy access to the currently running continuation object allows us to store the suspend arguments in there. However, there are a series of efficiency trade-offs: The "global" buffer currently used for suspend arguments can be re-used between different suspends, only being re-allocated whenever it is too small to hold the necessary number of suspend arguments. It also makes the current (rather naive) implementation of effect forwarding more efficient: While buffers local to continuation objects have to be handed over from parent to child (see the cont_obj_forward_tag_return_values_buffer builtin), this is not the case with the global payload buffer in the VMContext.

In total, I would suggest that we continue using the "global" buffer in the VMContext for suspend arguments for the time being, and revisit a better design once we either optimised effect forwarding or implement some kind of pooling logic for continuation objects and/or the buffers therein.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to not mangle with the VMContext too much. I agree that it would be more inefficient in the case of forwarding, but that's only because forwarding is implemented in a particularly naive way right now. Thus I don't worry about that at all.

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, I am not suggesting we do this refactor as part of this PR.

crates/runtime/src/continuation.rs Outdated Show resolved Hide resolved
@dhil
Copy link
Member

dhil commented Sep 21, 2023

I'll merge this one tomorrow

@frank-emrich
Copy link
Author

Okay, I've addressed two of your comments, let's leave the remaining two as future improvements.

@dhil dhil merged commit 031d2f1 into wasmfx:main Sep 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants