8000 Optimize coro::task by a858438680 · Pull Request #194 · jbaldwin/libcoro · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Optimize coro::task #194

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 24, 2023
Merged

Optimize coro::task #194

merged 2 commits into from
Oct 24, 2023

Conversation

a858438680
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author
a858438680 commented Oct 22, 2023 •
edited
Loading

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
Copy link
Owner
jbaldwin commented Oct 24, 2023 •
edited
Loading

Agreed, storage_size is clearly used 😅 . 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.

@jbaldwin jbaldwin merged commit 2b68298 into jbaldwin:main Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaldwin jbaldwin jbaldwin approved these changes

Assignees

@a858438680 a858438680

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@a858438680 @jbaldwin
0