Cleaning up coroutine implementation #3126
Conversation
|
As said on IRC, I'm slightly worried that this PR removes the previously existing symmetry between the functionality provided by yielding a thread and terminating a thread. While yielding still does allow for specifying another thread-id to the scheduler, instructing it to run the given thread first, this is not possible for terminating threads anymore (i.e. terminating threads do not allow to specify a continuation thread anymore). |
sithhell
force-pushed
the
coroutine_cleanup
branch
from
7354aea
to
6f3f08b
Compare
|
I've put the thread function returns back in. |
| @@ -517,7 +516,7 @@ namespace hpx { namespace threads | |||
| coroutine_type::result_type operator()() | |||
| { | |||
| HPX_ASSERT(this == coroutine_.get_thread_id().get()); | |||
| return coroutine_(set_state_ex(wait_signaled)); | |||
| return coroutine_(); | |||
There was a problem hiding this comment.
Shouldn't set_state_ex(wait_signaled) still be called here anyways (before the call to the coroutine)?
There was a problem hiding this comment.
There are currently 3 state transitions:
- (initialized) -> wait_signaled (not a state transition): a new thread is always put into the wait_signaled state
- wait_signaled -> wait_abort (to cancel a thread): No need to set it back to wait_signaled since the task will terminate
- wait_timeout -> wait_signaled (when a thread was woken up after a timeout): This is correctly handled here: https://github.com/STEllAR-GROUP/hpx/pull/3126/files#diff-1495f49124e718a7579611b3ba828883L325
I don't see a reason to reset the state_ex at each coroutine invocation.
There was a problem hiding this comment.
Ok, makes sense. Thanks for the explanations.
| } | ||
| catch (boost::exception const&) { | ||
| status = super_type::ctx_exited_abnormally; | ||
| tinfo = std::current_exception(); | ||
| this->reset(); |
There was a problem hiding this comment.
I'd suggest adding an assert here making sure the returned state == terminated.
| @@ -94,7 +92,7 @@ struct hpx_driver : htts2::driver | |||
| using hpx::util::placeholders::_1; | |||
There was a problem hiding this comment.
The using declaration could be removed now.
- Removing dynamic memory allocation - Removing intrusive ptr - Removing unused functions - Simplifying thread functions - Using thread_data directly to get/set the state_ex value
sithhell
force-pushed
the
coroutine_cleanup
branch
from
6f3f08b
to
9e1648c
Compare
|
@hkaiser Thanks for the heads up. I'll go ahead and merge this since this would be a very nice improvement to have in the release, but I'll revert if there are any signs of trouble. Please don't merge other PRs while this is being tested on buildbot. |
This patch reverts the changes related to the thread state_ex changes as they seem to lead to overall problems.
This patch reverts the changes related to the thread state_ex changes as they seem to lead to overall problems.
This patch reverts the changes related to the thread state_ex changes as they seem to lead to overall problems.
Proposed Changes
Any background context you want to provide?
This patch both simplifies and cleans up current code. This patch gives a rough speedup of about 1.5x for small grain sizes leading to heavy contention in the thread scheduler.
There is a potential breaking change for third party code using thread functions directly. I think this code is minimal and should be mostly under our control.