From 9f10f30a865fed6263e5f61b7cd8a88f8f6f43cd Mon Sep 17 00:00:00 2001 From: Audrow Date: Wed, 12 Aug 2020 15:35:39 -0700 Subject: [PATCH 1/8] Add document Signed-off-by: Audrow --- .../doc/multi_threaded_executor_redesign.rst | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 rclcpp/doc/multi_threaded_executor_redesign.rst diff --git a/rclcpp/doc/multi_threaded_executor_redesign.rst b/rclcpp/doc/multi_threaded_executor_redesign.rst new file mode 100644 index 0000000000..ce7ee0bf62 --- /dev/null +++ b/rclcpp/doc/multi_threaded_executor_redesign.rst @@ -0,0 +1,60 @@ +====================================== +:code:`MultiThreadedExecutor` Redesign +====================================== + +The :code:`MultiThreadedExecutor` may attempt to execute different types of executables multiple times. +This occurs because one executable object can be identified as ready in two threads and both threads will attempt to execute it. +To fix this, a general solution should do the following: + * Work for all the types of executable objects: timers, subscriptions, services, clients, and waitables + * Avoid spurious wake ups (which can be costly) + +Summary of discussion so far +---------------------------- + +Broadly speaking, two approaches were discussed: + +1. **Handle no data in the executor**: If more than one thread is trying to execute the same object and that object is consumed after it is executed once, check for the executable before trying to execute it: + + .. code-block:: c++ + + void execute() { + if(buffer->has_data()) { + auto data = buffer->consume_data(); + run_any_callback(data); + } else { + // do nothing + } + } + + :Advantages: + * simplicity + * doesn't extend interface in a way that doesn't generalize well for all other types of executable objects (i.e., timers) + :Disadvantages: + * spurious wake up calls + * needs to be documented well so every class that executes :code:`AnyExecutable` objects is thread-safe and doesn't hide a race condition + + +2. **Take the data**: When an executable is ready, take it and store it so that it can be executed when execute is called: + + .. code-block:: c++ + + void take_data(std::shared_ptr & data) { + data = buffer->consume_data(); + } + + void execute(std::shared_ptr & data) { + run_any_callback(data_); + } + + :Advantages: + * no spurious wake ups + :Disadvantages: + * more complex + * extends the interface in a way that doesn't generalize well for all other types of executable objects (i.e., timers) + * type erasure of data + +Ideas +----------------------- + * Add a preparation method to get the event after :code:`take_data` + * Take a lambda that can be executed later + * Store conditions in the waitset (like DDS) From 8d787b423c59da36fa5fe1a0dd14895a002d5839 Mon Sep 17 00:00:00 2001 From: Audrow Date: Wed, 12 Aug 2020 15:48:35 -0700 Subject: [PATCH 2/8] Fix indents Signed-off-by: Audrow --- rclcpp/doc/multi_threaded_executor_redesign.rst | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/rclcpp/doc/multi_threaded_executor_redesign.rst b/rclcpp/doc/multi_threaded_executor_redesign.rst index ce7ee0bf62..928a0a8185 100644 --- a/rclcpp/doc/multi_threaded_executor_redesign.rst +++ b/rclcpp/doc/multi_threaded_executor_redesign.rst @@ -5,8 +5,9 @@ The :code:`MultiThreadedExecutor` may attempt to execute different types of executables multiple times. This occurs because one executable object can be identified as ready in two threads and both threads will attempt to execute it. To fix this, a general solution should do the following: - * Work for all the types of executable objects: timers, subscriptions, services, clients, and waitables - * Avoid spurious wake ups (which can be costly) + +* Work for all the types of executable objects: timers, subscriptions, services, clients, and waitables +* Avoid spurious wake ups (which can be costly) Summary of discussion so far ---------------------------- @@ -55,6 +56,7 @@ Broadly speaking, two approaches were discussed: Ideas ----------------------- - * Add a preparation method to get the event after :code:`take_data` - * Take a lambda that can be executed later - * Store conditions in the waitset (like DDS) + +* Add a preparation method to get the event after :code:`take_data` +* Take a lambda that can be executed later +* Store conditions in the waitset (like DDS) From 63017ee5488a5960e1d3251e980964c874b9356a Mon Sep 17 00:00:00 2001 From: Audrow Date: Wed, 12 Aug 2020 16:03:56 -0700 Subject: [PATCH 3/8] Reword first sentences Signed-off-by: Audrow --- rclcpp/doc/multi_threaded_executor_redesign.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rclcpp/doc/multi_threaded_executor_redesign.rst b/rclcpp/doc/multi_threaded_executor_redesign.rst index 928a0a8185..663926df96 100644 --- a/rclcpp/doc/multi_threaded_executor_redesign.rst +++ b/rclcpp/doc/multi_threaded_executor_redesign.rst @@ -2,8 +2,8 @@ :code:`MultiThreadedExecutor` Redesign ====================================== -The :code:`MultiThreadedExecutor` may attempt to execute different types of executables multiple times. -This occurs because one executable object can be identified as ready in two threads and both threads will attempt to execute it. +:code:`MultiThreadedExecutor` may attempt to execute one executable object multiple times. +This occurs because an executable object can be identified as ready in multiple threads before it has been executed, and each of these threads will then try to execute it. To fix this, a general solution should do the following: * Work for all the types of executable objects: timers, subscriptions, services, clients, and waitables From e46398f6710f7f4551d95e81aa066bb12e938299 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Wed, 12 Aug 2020 18:55:01 -0700 Subject: [PATCH 4/8] Make wording clearer Signed-off-by: Audrow Nash Co-authored-by: Geoffrey Biggs --- rclcpp/doc/multi_threaded_executor_redesign.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/doc/multi_threaded_executor_redesign.rst b/rclcpp/doc/multi_threaded_executor_redesign.rst index 663926df96..bb6dac3c1e 100644 --- a/rclcpp/doc/multi_threaded_executor_redesign.rst +++ b/rclcpp/doc/multi_threaded_executor_redesign.rst @@ -29,7 +29,7 @@ Broadly speaking, two approaches were discussed: :Advantages: * simplicity - * doesn't extend interface in a way that doesn't generalize well for all other types of executable objects (i.e., timers) + * extends interface in a way that generalizes well for all other types of executable objects (i.e., timers) :Disadvantages: * spurious wake up calls * needs to be documented well so every class that executes :code:`AnyExecutable` objects is thread-safe and doesn't hide a race condition From e17e1f796d560e4536f947444a1efd78a9eb94c6 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Sep 2020 12:11:39 -0700 Subject: [PATCH 5/8] Remove spaces at ends of lines Signed-off-by: Audrow Nash --- rclcpp/doc/multi_threaded_executor_redesign.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rclcpp/doc/multi_threaded_executor_redesign.rst b/rclcpp/doc/multi_threaded_executor_redesign.rst index bb6dac3c1e..3e4c095ce7 100644 --- a/rclcpp/doc/multi_threaded_executor_redesign.rst +++ b/rclcpp/doc/multi_threaded_executor_redesign.rst @@ -2,7 +2,7 @@ :code:`MultiThreadedExecutor` Redesign ====================================== -:code:`MultiThreadedExecutor` may attempt to execute one executable object multiple times. +:code:`MultiThreadedExecutor` may attempt to execute one executable object multiple times. This occurs because an executable object can be identified as ready in multiple threads before it has been executed, and each of these threads will then try to execute it. To fix this, a general solution should do the following: @@ -14,7 +14,7 @@ Summary of discussion so far Broadly speaking, two approaches were discussed: -1. **Handle no data in the executor**: If more than one thread is trying to execute the same object and that object is consumed after it is executed once, check for the executable before trying to execute it: +1. **Handle no data in the executor**: If more than one thread is trying to execute the same entity and that entity is consumed after it is executed once, check for the executable before trying to execute it: .. code-block:: c++ @@ -27,10 +27,10 @@ Broadly speaking, two approaches were discussed: } } - :Advantages: + :Advantages: * simplicity * extends interface in a way that generalizes well for all other types of executable objects (i.e., timers) - :Disadvantages: + :Disadvantages: * spurious wake up calls * needs to be documented well so every class that executes :code:`AnyExecutable` objects is thread-safe and doesn't hide a race condition From 21b76281fb13e26dbdefc5e4c3aede99b244d912 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Sep 2020 12:12:15 -0700 Subject: [PATCH 6/8] Add clarification that the second method doesn't fix timers Signed-off-by: Audrow Nash --- rclcpp/doc/multi_threaded_executor_redesign.rst | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rclcpp/doc/multi_threaded_executor_redesign.rst b/rclcpp/doc/multi_threaded_executor_redesign.rst index 3e4c095ce7..b85367b482 100644 --- a/rclcpp/doc/multi_threaded_executor_redesign.rst +++ b/rclcpp/doc/multi_threaded_executor_redesign.rst @@ -35,20 +35,22 @@ Broadly speaking, two approaches were discussed: * needs to be documented well so every class that executes :code:`AnyExecutable` objects is thread-safe and doesn't hide a race condition -2. **Take the data**: When an executable is ready, take it and store it so that it can be executed when execute is called: +2. **Take the data**: This approach takes the data when collecting the ready entities, and that process is done in a mutually exclusive fashion. Thus, this approach may avoid spurious wake ups. .. code-block:: c++ + // called first void take_data(std::shared_ptr & data) { data = buffer->consume_data(); } + // called second void execute(std::shared_ptr & data) { run_any_callback(data_); } :Advantages: - * no spurious wake ups + * fixes spurious wake ups for events where :code:`take_data` has a meaning (i.e., not timers) :Disadvantages: * more complex * extends the interface in a way that doesn't generalize well for all other types of executable objects (i.e., timers) From 7315b380590e03cbd52b5149ce674d86acab9067 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Sep 2020 12:13:06 -0700 Subject: [PATCH 7/8] Add an example for returning a lambda function Signed-off-by: Audrow Nash --- .../doc/multi_threaded_executor_redesign.rst | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/rclcpp/doc/multi_threaded_executor_redesign.rst b/rclcpp/doc/multi_threaded_executor_redesign.rst index b85367b482..752bebdc5b 100644 --- a/rclcpp/doc/multi_threaded_executor_redesign.rst +++ b/rclcpp/doc/multi_threaded_executor_redesign.rst @@ -56,9 +56,26 @@ Broadly speaking, two approaches were discussed: * extends the interface in a way that doesn't generalize well for all other types of executable objects (i.e., timers) * type erasure of data +.. note:: + + To avoid erasing the type of the data, :code:`take_data` could be replaced with a :code:`get_handle` method that would return a lambda function. + + .. code-block:: c++ + + // called first + std::function get_handle() { + return [data=buffer->consume_data()]() { + run_any_callback(data); + }; + } + + // called second + void execute(std::function handle) { + handle(); + } + Ideas ----------------------- * Add a preparation method to get the event after :code:`take_data` -* Take a lambda that can be executed later * Store conditions in the waitset (like DDS) From f280c2cbf46404981767237be2dec1251ded9f11 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Sep 2020 12:14:20 -0700 Subject: [PATCH 8/8] Add an example for the preparation idea Signed-off-by: Audrow Nash --- .../doc/multi_threaded_executor_redesign.rst | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/rclcpp/doc/multi_threaded_executor_redesign.rst b/rclcpp/doc/multi_threaded_executor_redesign.rst index 752bebdc5b..75bfa37584 100644 --- a/rclcpp/doc/multi_threaded_executor_redesign.rst +++ b/rclcpp/doc/multi_threaded_executor_redesign.rst @@ -77,5 +77,26 @@ Broadly speaking, two approaches were discussed: Ideas ----------------------- -* Add a preparation method to get the event after :code:`take_data` +* Add a preparation method to get the event after :code:`take_data`: + + .. code-block:: c++ + + std::shared_ptr & data_; + + // called first + void take_data() { + data_ = buffer->consume_data(); + } + + // called second + std::shared_ptr get_prepared() { + return data_; + } + + // called third, with the data returned in the previous step + void execute(std::shared_ptr & data) { + run_any_callback(data); + } + + * Store conditions in the waitset (like DDS)