[try_put_and_wait] Part 2: Add implementation of try_put_and_wait feature for buffering nodes #1412
Conversation
kboyarinov
requested review from
aleksei-fedotov,
vossmjp,
pavelkumbrasev,
dnmokhov and
isaevil
kboyarinov
changed the title
kboyarinov
force-pushed
the
dev/kboyarinov/try_put_and_wait_buffering_nodes
branch
from
50d92c0 to
ae591d5
Compare
kboyarinov
force-pushed
the
dev/kboyarinov/try_put_and_wait_buffering_nodes
branch
from
eb18ebe to
811b9b9
Compare
Co-authored-by: Aleksei Fedotov <[email protected]>
…function_node' into dev/kboyarinov/try_put_and_wait_buffering_nodes
| bool get_item( output_type& v ) { | ||
| return get_item_impl(v); | ||
| } | ||
|
|
||
| #if __TBB_PREVIEW_FLOW_GRAPH_TRY_PUT_AND_WAIT | ||
| bool get_item( output_type& v, message_metainfo& metainfo ) { | ||
| return get_item_impl(v, &metainfo); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Does it make sense to make it a single method as get_item_impl? (may be for consistency)
There was a problem hiding this comment.
I am not sure I understood correctly. get_item should be considered a public API for a cache. get_item_impl - as a private method. We need to keep the existing public API get_item(v) and add a new one get_item(v, metainfo) and implement both of them on top of get_item_impl.
There was a problem hiding this comment.
I suppose for try_put_task, there are calls like try_put_task_impl(t __TBB_FLOW_GRAPH_METAINFO_ARG(message_metainfo{})). But for get_item, you'd need to create a temporary that is just dropped. While for try_put_task you still want to send the empty metainfo. Is this the reason there are two get_item_impl functions instead of something like the single try_put_task_impl function?
| bool get_item( output_type& v ) { | ||
| return get_item_impl(v); | ||
| } | ||
|
|
||
| #if __TBB_PREVIEW_FLOW_GRAPH_TRY_PUT_AND_WAIT | ||
| bool get_item( output_type& v, message_metainfo& metainfo ) { | ||
| return get_item_impl(v, &metainfo); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I suppose for try_put_task, there are calls like try_put_task_impl(t __TBB_FLOW_GRAPH_METAINFO_ARG(message_metainfo{})). But for get_item, you'd need to create a temporary that is just dropped. While for try_put_task you still want to send the empty metainfo. Is this the reason there are two get_item_impl functions instead of something like the single try_put_task_impl function?
…production' into dev/kboyarinov/try_put_and_wait_function_node
…function_node' into dev/kboyarinov/try_put_and_wait_buffering_nodes
…production' into dev/kboyarinov/try_put_and_wait_buffering_nodes
| Args... args) | ||
| { | ||
| std::size_t after_try_put_and_wait_start_index = 0; | ||
| tbb::task_arena arena(1); |
There was a problem hiding this comment.
I see that all of the tests are ultimately completely serial. The function_node is serial but also the flow graph executes in an arena with a single slot. I suppose this is done to make the results deterministic since most of the tests are whitebox tests that know the exact processing order. Is there risk that serial execution could hide some bugs caused by concurrency?
| #if __TBB_PREVIEW_FLOW_GRAPH_TRY_PUT_AND_WAIT | ||
| void fetch_item(size_t i, item_type& o, message_metainfo& metainfo) { | ||
| __TBB_ASSERT(my_item_valid(i), "Trying to fetch an empty slot"); | ||
| o = get_my_item(i); // could have std::move assign semantics |
There was a problem hiding this comment.
When __TBB_PREVIEW_FLOW_GRAPH_TRY_PUT_AND_WAIT is defined, is the other overload of this function ever called? I could only find one use in flow_graph.h and it used the _ARG macro. I suppose you could have #if - #else if that's true. Seems like you only need one function or the other, never both.
| #if __TBB_PREVIEW_FLOW_GRAPH_TRY_PUT_AND_WAIT | ||
| buffer_operation(const T& e, op_type t, const message_metainfo& info) | ||
| : type(char(t)), elem(const_cast<T*>(&e)), ltask(nullptr), r(nullptr) | ||
| , metainfo(const_cast<message_metainfo*>(&info)) |
There was a problem hiding this comment.
Such pattern of const-casting can lead to dangling references. Can we accept message_metainfo without cv-qualifiers in the first place?
There was a problem hiding this comment.
Since metainfo is taken as constant reference in try_put_task virtual method, we will still need to const_cast it somewhere to store it into the buffer_operation. So I would prefer to keep all casts in one class.
kboyarinov
merged commit 5cd159a
into
dev/kboyarinov/try_put_and_wait_production
Description
Extend buffer_node, queue_node, priority_queue_node and sequencer_node interfaces with the new try_put_and_wait API for waiting on single messages.
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