Add async stack support to coroutines #632
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
ispeters
force-pushed
the
coroutine_async_stack_support
branch
3 times, most recently
from
48f411c
to
e8a18b9
Compare
ispeters
force-pushed
the
coroutine_async_stack_support
branch
from
e8a18b9
to
fe4ab6c
Compare
ispeters
force-pushed
the
tweak_any_sender_of_opstate
branch
from
b63a902
to
4e4f5e9
Compare
ispeters
force-pushed
the
coroutine_async_stack_support
branch
from
fe4ab6c
to
45ca45b
Compare
This PR continues what was started in #635 and adds more customizations of `get_return_address`, starting with `task<>`.
ispeters
force-pushed
the
coroutine_async_stack_support
branch
from
45ca45b
to
bdefd9f
Compare
This change extends the work in #616 to support async stack frames in `task<>` coroutines, including those that invoke `at_coroutine_exit()`. In `task<>`, when `UNIFEX_NO_ASYNC_STACKS` is falsey, the awaiter returned from `task<>`'s customization of `unifex::await_transform` stores an `AsyncStackFrame`. The awaiter pushes its frame onto the current async stack in `await_suspend()` and pops it again in `await_resume()`; since `await_resume()` is only invoked for value and error completions, this arrangement leaves it up to the waiting task to pop the awaiter's frame when the awaited task completes with done. This can be expressed as a new rule: - when a coroutine completes with a value or an error, it is responsible for popping its own `AsyncStackFrame`; but - when a coroutine completes with done, the *caller* is responsible for popping the callee's `AsyncStackFrame` as a part of the caller's `unhandled_done()` coroutine. To support this new requirement of `unhandled_done()` (that it is responsible for popping the callee's stack frame), this change introduces `popAsyncStackFrameFromCaller`, which takes the caller's stack frame by reference so that it can assert that, after popping the current async frame (whatever it is), the new top frame is the caller's frame. A `task<>` promise has an `AsyncStackFrame*` that, when it's not `nullptr`, points to the `AsyncStackFrame` in the awaiter waiting for the task. This pointer exists even when `UNIFEX_NO_ASYNC_STACKS` is truthy to help mitigate against ODR violations; linking together two TUs with `UNIFEX_NO_ASYNC_STACKS` set differently is not explicitly supported but, by ensuring this pointer always exists, some ODR problems are avoided. When a `task<>` is awaited from a TU with async stack support enabled, the awaited task's awaiter sets the promise's `AsyncStackFrame*` to point to the awaiter's frame; when a `task<>` is awaited from a TU with async stack support disabled, this assignment never happens and the promise's pointer remains null. The above description of `task<>`'s async stack maintenance only covers the recursive case of on coroutine awaiting another. The base case is handled in `connect_awaitable()`, where an `AsyncStackRoot` is set up before starting the connected awaitable. `stop_if_requested` used to model both `sender` and `awaitable` so that `co_await stop_if_requested();` could take advantage of symmetric transfer. The `stop_if_requested` sender now customizes `await_transform` to express its participation in async stack management. This means of expressing async stack awareness is unsatisfying but I don't have any better ideas right now. Lastly, `unifex::await_transform()` now wraps naturally-awaitable arguments in an `awaiter_wrapper` that ensures the `coroutine_handle<>` passed to the wrapped awaitable is one that establishes an active `AsyncStackRoot` before resuming the real waiting coroutine.
ispeters
force-pushed
the
coroutine_async_stack_support
branch
from
987a18c
to
bbca487
Compare
jesswong
reviewed
Sep 26, 2024
Comment on lines
+117
to
+122
| if constexpr ( | ||
| WithAsyncStackSupport && same_as<CPO, tag_t<get_async_stack_frame>>) { | ||
| return &p.frame_; | ||
| } else { | ||
| return std::move(cpo)(std::as_const(p.receiver_)); | ||
| } |
There was a problem hiding this comment.
this is a weird check. is it possible to separately define a get_async_stack_frame tag_invoke that will be called instead of this generic CPO function?
There was a problem hiding this comment.
It is weird, but I ran into trouble (that I don't remember in detail now) expressing the conditions correctly on a separate tag_invoke overload.
| @@ -297,6 +365,7 @@ struct _fn { | |||
| (requires detail::_awaitable<Awaitable>) // | |||
| _sender<remove_cvref_t<Awaitable>> | |||
| operator()(Awaitable&& awaitable) const { | |||
| // TODO: this is going to generate an unfortunate return address | |||
There was a problem hiding this comment.
Because it's inside the bowels of Unifex here; I think a better return address might be get_return_address(awaitable). We might have actually changed this in one of the branches that improves return address reporting for task<>.
janondrusek
approved these changes
Sep 26, 2024
| @@ -144,7 +157,7 @@ struct _fn final { | |||
| if constexpr ( | |||
| !same_as<blocking_kind, blocking_t> && | |||
| (blocking_kind::always_inline == blocking_t{})) { | |||
| return Awaitable{(Awaitable &&) awaitable}; | |||
| return Awaitable{(Awaitable&&)awaitable}; | |||
There was a problem hiding this comment.
nit: wanna rid of the C-style cast?
There was a problem hiding this comment.
I do want to, but the clock has run out.
Comment on lines
+72
to
+73
| auto root = tryGetCurrentAsyncStackRoot(); | ||
| assert(root != nullptr); |
There was a problem hiding this comment.
why call it try if it cannot be nullptr?
There was a problem hiding this comment.
The current async stack root is allowed to be nullptr in general, but the stack is in a bad state if it's null in this case. The alternative way to read the current stack root is getCurrentAsyncStackRoot(), but that returns a reference; if the stack is corrupt at this point, we can't meaningfully assert that a reference is valid, but we can assert that a nullable pointer isn't null.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change extends the work in #616 to support async stack frames in
task<>coroutines, including those that invokeat_coroutine_exit().In
task<>, whenUNIFEX_NO_ASYNC_STACKSis falsey, the awaiter returned fromtask<>'s customization ofunifex::await_transformstores anAsyncStackFrame. The awaiter pushes its frame onto the current async stack inawait_suspend()and pops it again inawait_resume(); sinceawait_resume()is only invoked for value and error completions, thisarrangement leaves it up to the waiting task to pop the awaiter's frame
when the awaited task completes with done. This can be expressed as a
new rule:
for popping its own
AsyncStackFrame; butpopping the callee's
AsyncStackFrameas a part of the caller'sunhandled_done()coroutine.To support this new requirement of
unhandled_done()(that it isresponsible for popping the callee's stack frame), this change
introduces
popAsyncStackFrameFromCaller, which takes the caller'sstack frame by reference so that it can assert that, after popping the
current async frame (whatever it is), the new top frame is the caller's
frame.
A
task<>promise has anAsyncStackFrame*that, when it's notnullptr, points to theAsyncStackFramein the awaiter waiting forthe task. This pointer exists even when
UNIFEX_NO_ASYNC_STACKSistruthy to help mitigate against ODR violations; linking together two TUs
with
UNIFEX_NO_ASYNC_STACKSset differently is not explicitlysupported but, by ensuring this pointer always exists, some ODR problems
are avoided. When a
task<>is awaited from a TU with async stacksupport enabled, the awaited task's awaiter sets the promise's
AsyncStackFrame*to point to the awaiter's frame; when atask<>isawaited from a TU with async stack support disabled, this assignment
never happens and the promise's pointer remains null.
The above description of
task<>'s async stack maintenance only coversthe recursive case of on coroutine awaiting another. The base case is
handled in
connect_awaitable(), where anAsyncStackRootis set upbefore starting the connected awaitable.
stop_if_requestedused to model bothsenderandawaitableso thatco_await stop_if_requested();could take advantage of symmetrictransfer. The
stop_if_requestedsender now customizesawait_transformto express its participation in async stackmanagement. This means of expressing async stack awareness is
unsatisfying but I don't have any better ideas right now.
Lastly,
unifex::await_transform()now wraps naturally-awaitablearguments in an
awaiter_wrapperthat ensures thecoroutine_handle<>passed to the wrapped awaitable is one that establishes an active
AsyncStackRootbefore resuming the real waiting coroutine.