search

jbaldwin / libcoro

C++20 coroutine library
Apache License 2.0
599 stars 62 forks source link

Optimize coro::task #194

Closed a858438680 closed 1 year ago

a858438680 commented 1 year ago

Please refer to issue #193 .

This pull request does lots of optimization including

  1. sharing storage between value and exception ptr.
  2. allowing inplace construction inside the promise to reduce 1 move/copy
  3. disabling all move/copy ctor/assign functions
  4. adding test to make sure that non assignable types can be used to initialize the coro::task
jbaldwin commented 1 year ago

I've taken a cursory look and it looks awesome. I'm going to do a more thorough review tomorrow when I have some time to fully digest the change.

Can you look at the codacy item it flagged and see if it's a good change?

a858438680 commented 1 year ago

I've taken a cursory look and it looks awesome. I'm going to do a more thorough review tomorrow when I have some time to fully digest the change.

Can you look at the codacy item it flagged and see if it's a good change?

Of course, I have already looked at it and in the first item, it says that the storage_size is not used, which is actually not the truth. The storage_size is a constexpr variable and is used as the size of member variable m_storage. To be truth, I think this is bug of Codacy and someone should create an new issue to tell them. 😂

In the second item, it says that the m_storage is not initialized, however this is by design, because, I do not want it to be initialized to zero, because this is not necessary and introduces semantic noises. When the task is created, the m_storage is uninitialized, but the only way to access this m_storage by the user is to call result(), however it checks the m_flag. When it is empty, the result() throws an exception. So this m_storage is safely to be uninitialized.

jbaldwin commented 1 year ago

Agreed, storage_size is clearly used :sweat_smile: . I'll mark that one as wont fix (assuming I can..).

Agreed on part 2 as well, it doesn't need to be zero initialized when its empty/just created. I'll try and mark it as won't fix as well.

Very nice change, I gave it a proper good look and I'm excited and glad you pushed this PR, its a great addition to making the task class footprint smaller. Thank you.