[try_put_and_wait] Add tag-based implementation for try_put_and_wait for multifunction and async nodes #1604
Conversation
kboyarinov
requested review from
aleksei-fedotov,
vossmjp,
pavelkumbrasev,
dnmokhov and
isaevil
| @@ -0,0 +1,468 @@ | |||
| /* | |||
There was a problem hiding this comment.
Most part of this file was moved from test_buffering_try_put_and_wait without any changes
|
Some of TODOs are still required to be closed before merging this patch, I will do it in the later updates |
|
|
||
| metainfo_tag_type& operator=(const metainfo_tag_type&) = delete; | ||
| metainfo_tag_type& operator=(metainfo_tag_type&& other) { | ||
| // TODO: should this method be thread-safe? |
There was a problem hiding this comment.
Could someone want to use assignment in place of reset to start a next batch of accumulations -- starting with a different first tag. Then there could be a race between the assignment and merge..?
There was a problem hiding this comment.
It is in theory really interesting, which thread-safety guarantees should be provided.
It is obvious, that:
mergeshould be thread-safe with othermergeresetshould be thread-safe withreset
merge can be thread-safe with reset (as well as with operator=) but it is hard to imagine the real use case. If we have both merge and reset simultaneously, which result can the user expect - merged or reset object.
Currently, the first function that acquires the mutex will define the behavior.
But let's consider the 2 consecutive reductions that use the single accumulators. If in the edge between 2 reductions, there would be simultaneous merge from reduction 2 and reset from the reduction 1, some elements from the reduction 2 can be missed if the merge will go before the reset.
For me, it is unclear how we can manage this. I think the answer for this question is the answer also to thread-safety of operator=
There was a problem hiding this comment.
Other interesting question is should it be thread-safe to have the left.merge(middle) simultaneous with middle.merge(right). I feel that the answer is no, but still want to highlight this.
| @@ -657,7 +657,7 @@ void test_try_put_and_wait_rejecting(size_t concurrency_limit) { | |||
| std::vector<int> processed_items; | |||
| std::vector<int> new_work_items; | |||
|
|
|||
| int wait_message = 0; | |||
| int wait_message = 10; | |||
There was a problem hiding this comment.
You forgot to update copyright year.
Description
Add reference implementation of try_put_and_wait feature for multifunction and async nodes as described in the corresponding RFC.
Fixes # - issue number(s) if exists
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@to send notificationsOther information