Skip to content

Commit

Permalink
Simplify ExtendedCoroutinePromise interface
Browse files Browse the repository at this point in the history
Summary:
- I see no benefit to `getHandle()` being part of the virtual inteface. The code certainly gets shorter!
- Remove `ExtendedCoroutineHandle::getPromise` -- it's is unused, and of dubious value
- Mark `getErrorHandle` as `final` throughout, the goal being that removing `final` from `TaskPromise` should not impact compiler optimizations since those classes would remain non-polymorphic.

Reviewed By: iahs

Differential Revision: D61404120

fbshipit-source-id: a337a57eb4aa1bdf34e98af527a28331cc0414f9
  • Loading branch information
Alexey Spiridonov authored and facebook-github-bot committed Aug 28, 2024
1 parent 4db2eb2 commit 7dc0554
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 32 deletions.
8 changes: 3 additions & 5 deletions folly/coro/AsyncGenerator.h
Original file line number Diff line number Diff line change
Expand Up @@ -486,10 +486,8 @@ struct BaseAsyncGeneratorPromise<true> {
};

template <typename Reference, typename Value, bool RequiresCleanup = false>
class AsyncGeneratorPromise final
: public ExtendedCoroutinePromiseImpl<
AsyncGeneratorPromise<Reference, Value, RequiresCleanup>>,
BaseAsyncGeneratorPromise<RequiresCleanup> {
class AsyncGeneratorPromise final : public ExtendedCoroutinePromise,
BaseAsyncGeneratorPromise<RequiresCleanup> {
class YieldAwaiter {
public:
bool await_ready() noexcept { return false; }
Expand Down Expand Up @@ -720,7 +718,7 @@ class AsyncGeneratorPromise final
folly::AsyncStackFrame& getAsyncFrame() noexcept { return asyncFrame_; }

std::pair<ExtendedCoroutineHandle, AsyncStackFrame*> getErrorHandle(
exception_wrapper& ex) override {
exception_wrapper& ex) final {
if (bypassExceptionThrowing_ == BypassExceptionThrowing::ACTIVE) {
auto yieldAwaiter = yield_value(co_error(std::move(ex)));
DCHECK(!yieldAwaiter.await_ready());
Expand Down
29 changes: 7 additions & 22 deletions folly/coro/Coroutine.h
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,6 @@ class ExtendedCoroutineHandle;
// Extended promise interface folly::coro types are expected to implement
class ExtendedCoroutinePromise {
public:
virtual coroutine_handle<> getHandle() = 0;
// Types may provide a more efficient resumption path when they know they will
// be receiving an error result from the awaitee.
// If they do, they might also update the active stack frame.
Expand All @@ -323,8 +322,13 @@ class ExtendedCoroutineHandle {
/*implicit*/ ExtendedCoroutineHandle(coroutine_handle<> handle) noexcept
: basic_(handle) {}

/*implicit*/ ExtendedCoroutineHandle(ExtendedCoroutinePromise* ptr) noexcept
: basic_(ptr->getHandle()), extended_(ptr) {}
template <
typename Promise,
std::enable_if_t<
std::is_base_of_v<ExtendedCoroutinePromise, Promise>,
int> = 0>
/*implicit*/ ExtendedCoroutineHandle(Promise* p) noexcept
: basic_(coroutine_handle<Promise>::from_promise(*p)), extended_(p) {}

ExtendedCoroutineHandle() noexcept = default;

Expand All @@ -334,8 +338,6 @@ class ExtendedCoroutineHandle {

coroutine_handle<> getHandle() const noexcept { return basic_; }

ExtendedCoroutinePromise* getPromise() const noexcept { return extended_; }

std::pair<ExtendedCoroutineHandle, AsyncStackFrame*> getErrorHandle(
exception_wrapper& ex) {
if (extended_) {
Expand All @@ -360,23 +362,6 @@ class ExtendedCoroutineHandle {
ExtendedCoroutinePromise* extended_{nullptr};
};

template <typename Promise>
class ExtendedCoroutinePromiseImpl : public ExtendedCoroutinePromise {
public:
coroutine_handle<> getHandle() final {
return coroutine_handle<Promise>::from_promise(
*static_cast<Promise*>(this));
}

std::pair<ExtendedCoroutineHandle, AsyncStackFrame*> getErrorHandle(
exception_wrapper&) override {
return {getHandle(), nullptr};
}

protected:
~ExtendedCoroutinePromiseImpl() = default;
};

} // namespace folly::coro

#endif
4 changes: 2 additions & 2 deletions folly/coro/Task.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ class TaskPromiseBase {
// Separate from `TaskPromiseBase` so the compiler has less to specialize.
template <typename Promise, typename T>
class TaskPromiseCrtpBase : public TaskPromiseBase,
public ExtendedCoroutinePromiseImpl<Promise> {
public ExtendedCoroutinePromise {
public:
using StorageType = detail::lift_lvalue_reference_t<T>;

Expand Down Expand Up @@ -232,7 +232,7 @@ class TaskPromiseCrtpBase : public TaskPromiseBase,
~TaskPromiseCrtpBase() = default;

std::pair<ExtendedCoroutineHandle, AsyncStackFrame*> getErrorHandle(
exception_wrapper& ex) override {
exception_wrapper& ex) final {
auto& me = *static_cast<Promise*>(this);
if (bypassExceptionThrowing_ == BypassExceptionThrowing::ACTIVE) {
auto finalAwaiter = yield_value(co_error(std::move(ex)));
Expand Down
6 changes: 3 additions & 3 deletions folly/coro/ViaIfAsync.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ template <bool IsStackAware>
class ViaCoroutine {
public:
class promise_type final : public ViaCoroutinePromiseBase,
public ExtendedCoroutinePromiseImpl<promise_type> {
public ExtendedCoroutinePromise {
struct FinalAwaiter {
bool await_ready() noexcept { return false; }

Expand Down Expand Up @@ -154,13 +154,13 @@ class ViaCoroutine {
folly::AsyncStackFrame& getLeafFrame() noexcept { return leafFrame_; }

std::pair<ExtendedCoroutineHandle, AsyncStackFrame*> getErrorHandle(
exception_wrapper& ex) override {
exception_wrapper& ex) final {
auto [handle, frame] = continuation_.getErrorHandle(ex);
setContinuation(handle);
if (frame && IsStackAware) {
leafFrame_.setParentFrame(*frame);
}
return {promise_type::getHandle(), nullptr};
return {coroutine_handle<promise_type>::from_promise(*this), nullptr};
}
};

Expand Down

0 comments on commit 7dc0554

Please sign in to comment.