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

Optimize co_return Future<V, E> from Future<V, E> #189

Open
MBkkt opened this issue Aug 6, 2022 · 4 comments
Open

Optimize co_return Future<V, E> from Future<V, E> #189

MBkkt opened this issue Aug 6, 2022 · 4 comments
Labels
good first issue Good for newcomers performance Improve performance

Comments

@MBkkt
Copy link
Member

MBkkt commented Aug 6, 2022

First of feel free to ask me(@MBkkt) any questions about this task, often the task may not be described in enough detail for devs who don't have the full context.

Sometimes we want to return one coroutine result from another coroutine (pseudo-code):

yaclib::Future<int> SomeAsync(...) {
  ... some async code ...
  yaclib::Future<int> f = yaclib::Run(...) // run another coroutine
  ... some async code ...
  co_return f;
}

Now it can be written as

yaclib::Future<int> SomeAsync(...) {
  ... some async code ...
  yaclib::Future<int> f = yaclib::Run(...) // run another coroutine
  ... some async code ...
  co_await Await(f);
  co_return std::move(f).Get();
}

After this task #187:

yaclib::Future<int> SomeAsync(...) {
  ... some async code ...
  yaclib::Future<int> f = yaclib::Run(...) // run another coroutine
  ... some async code ...
  co_return co_await std::move(f);
}

We cannot overload promise_type::return_value for this, because after compiler call return_value it call final_suspend, but we don't want it (because we want call SetResult only in Here call see below).

So we need to create special Awaiter for this task.

I think it can be created with function named yaclib::Return or yaclib::CoReturn:

yaclib::Future<int> SomeAsync(...) {
  ... some async code ...
  yaclib::Future<int> f = yaclib::Run(...) // run another coroutine
  ... some async code ...
  co_await Return(std::move(f));
  ... unreachable ...
}

Why is it better than 3rd code snippet?
Because we can make it cheaper:
In third snippet we create awaiter on stack set it as callback to f.
Then we suspend
After this callback will be called we resumed and make move value from f to our coroutine
and then we make final suspend.

With this suggestion we can make single suspend instead of two for this return.

How to make this Awaiter(pseudo-code)

struct ReturnAwaiter {
  explicit ReturnAwaiter(Future<T>&&);

  constexpr bool await_ready() const { return false; } // we want to make return so always false
  constexpr void await_resume() const {};

  template<typename Promise>
  void await_suspend(std::coroutine_handle<Promise> handle) {
    // here we set callback to future that we want to return.
    // we set our coroutine (handle) to callback
    // so we need to implement this callback for PromiseType<V, E>
    future.SetHere(handle.promise(), kHereCall); 
  }
};

How to implement callback for PromiseType<V, E>:
This callback named virtual void BaseCore::Here(...)

void PromiseType<V, E>::Here(...) noexcept final {
  this->Store(core.Get()); // we move value from future that we want return to our coroutine storage
  this->SetResult();          // we mark this coroutine as done
}

https://github.com/YACLib/YACLib/blob/main/include/yaclib/coroutine/detail/promise_type.hpp#L40

@MBkkt MBkkt added good first issue Good for newcomers performance Improve performance labels Aug 6, 2022
@MBkkt MBkkt added this to the Not main priority milestone Aug 7, 2022
@MBkkt
Copy link
Member Author

MBkkt commented Nov 15, 2022

Here/Next now should be implemented for ReturnAwaiter, not for PromiseType

@MBkkt
Copy link
Member Author

MBkkt commented Nov 15, 2022

Also same needs for Task

@chethan27
Copy link

@MBkkt would like to work on this

@MBkkt
Copy link
Member Author

MBkkt commented Feb 25, 2023

@chethan27 Hello, cool!
One moment. Now co_await single future was optimized.
So implementation suggested in this issue is impossible.

Instead you need to make Return awaiter a derived from InlineCore, and implement this Here/Next (as suggested in issue) for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers performance Improve performance
Projects
None yet
Development

No branches or pull requests

2 participants