add events-executor and timers-manager in rclcpp #2155

alsora · 2023-04-04T07:28:44Z

This PR introduces the events-executor based on the design here ros2/design#305 and the already-existing open-source implementation in https://github.com/irobot-ros/events-executor.

FYI @clalancette @wjwwood @mjcarroll @mauropasse @bpwilcox

This PR currently targets Michael's branch as it depends on it.
Will target the main branch after this PR is merged.
#2143

alsora · 2023-04-04T07:35:01Z

rclcpp/test/rclcpp/executors/test_events_executor.cpp

I think that most (all?) of the tests here can probably moved to the test_executors.cpp file as they are valid tests for all executors.

rclcpp/include/rclcpp/experimental/executors/events_executor/events_executor.hpp

rclcpp/include/rclcpp/experimental/executors/events_executor/events_executor_event_types.hpp

mauropasse · 2023-04-12T08:37:59Z

rclcpp/include/rclcpp/experimental/executors/events_executor/simple_events_queue.hpp

+private:
+  // The underlying queue implementation
+  std::queue<rclcpp::experimental::executors::ExecutorEvent> event_queue_;
+  // Mutex to protect the insertion/extraction of events in the queue


Is not just insertion/extraction, is also checking size and if empty:

Suggested change

// Mutex to protect the insertion/extraction of events in the queue

// Mutex to protect concurrent actions over the queue

rclcpp/include/rclcpp/experimental/timers_manager.hpp

rclcpp/src/rclcpp/experimental/timers_manager.cpp

mauropasse · 2023-04-12T09:51:08Z

rclcpp/src/rclcpp/executors/executor_notify_waitable.cpp

+  // The second argument of the callback should identify which guard condition
+  // triggered the event.
+  // We could indicate which of the guard conditions was triggered, but the executor
+  // is already going to check that.


Actually the executor doesn't check which guard condition was triggered, it just knows the waitable to whom it belongs.

If the executor knows which gc was triggered, it could more efficiently do work on the node or entity having this gc.
For example right know, a single gc triggered due addition of an entity to a node, will result on re-setting the callbacks to all nodes entities registered in the executor. We could use this info to get only the node having this gc and setting the callbacks to it. This is just a peformance improvement, but we already have the notify_waitable_event_pushed_ which is mitigating this issue.
Also if we manage to add apis to not notify the executor for every entity added, until manually done later, this will be less a problem

Few comments here:

Yes, the executor doesn't check which guard condition triggered, but we could easily do that (maybe I should change the should into could in the comment)

This new implementation does not re-set the callbacks to all the nodes. It performs a loop on all the entities to understand which ones have been added/removed and then it does only the necessary work. By taking into account the possible improvement mentioned in (1) we could speed up the loop (i.e. loop only on one node, rather than on all nodes), but the set/unset would remain equivalent.

IMO, the fact that we don't reset everything, plus the presence of the notify_waitable_event_pushed_ flag, should already improve the resource usage during startup.

Controlling the notification to the executor would still be important to reduce the amount of time we loop over the entities.

Also if we manage to add apis to not notify the executor for every entity added, until manually done later, this will be less a problem

While we can't get this for Iron, I would like to get it in sooner rather than later in rolling while we all still have context.

rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp

rclcpp/include/rclcpp/executors/executor_notify_waitable.hpp

mjcarroll · 2023-04-13T19:54:53Z

Linux
Linux-aarch64
Windows

alsora · 2023-04-14T12:41:34Z

Rebased with latest "exectuor_structures" branch;

fixed uncrustify and comments a642e4e
fixed some small issues if not using events-executor be41ece

Linux
Linux-aarch64
Windows

alsora · 2023-04-14T19:32:05Z

New CI

Linux
Linux-aarch64
Windows

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

fix check for execute_timers_separate_thread; ignore on_ready_callback if asked to execute a timer; reduce usage of void pointers Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

…ks only if necessary Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

wjwwood

lgtm

clalancette

This looks good to me. I'm going to run CI on this again since there have been host of changes since the last one. I'll also run Windows Debug and RHEL.

clalancette · 2023-04-17T14:21:20Z

CI:

Linux
Linux-aarch64
Windows
Windows Debug
RHEL

clalancette · 2023-04-17T18:33:29Z

All right, everything is green here so going ahead and merging. Thanks everyone for the reviews and work.

* add events-executor and timers-manager in rclcpp fix check for execute_timers_separate_thread; ignore on_ready_callback if asked to execute a timer; reduce usage of void pointers * have the executor notify waitable fiddle with guard condition callbacks only if necessary Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

tonynajjar · 2023-05-05T09:08:00Z

Any plans to backport this to Humble or is it too complex to do so?

alsora · 2023-05-05T13:48:45Z

We don't currently have plans to do it.
As a reminder, there's this repository with the events-executor code that is compatible with Humble https://github.com/irobot-ros/events-executor
(although I don't remember if Humble contains all the needed features or if you still would need to build it from sources and apply some patches).

The implementation of the events-executor that got merged in this PR is slightly different from what is in the old repo.
In particular, it uses new entity collection classes that just got developed for ROS 2 Iron.

If all the prerequisites are there already, it should be possible to backport without touching existing files, and I would be more than happy to review the PRs.

russkel · 2023-05-19T00:54:39Z

Would it be possible to get this into iron? Or is that something not done with non-LTS releases? Seems unfortunate to have to wait till next ROS2 release for this.

clalancette · 2023-05-19T11:51:25Z

Would it be possible to get this into iron? Or is that something not done with non-LTS releases? Seems unfortunate to have to wait till next ROS2 release for this.

This is currently in Iron.

russkel · 2023-05-21T06:54:28Z

Would it be possible to get this into iron? Or is that something not done with non-LTS releases? Seems unfortunate to have to wait till next ROS2 release for this.

This is currently in Iron.

Ah great. I must have missed it in the release notes.

tonynajjar · 2023-05-21T07:05:48Z

I think it's not in the release notes, at least it wasn't 5 days ago when I looked. It would be useful to add! (+ How to make the switch)

clalancette · 2023-05-23T03:12:24Z

@mjcarroll is adding it in ros2/ros2_documentation#3662

* add events-executor and timers-manager in rclcpp fix check for execute_timers_separate_thread; ignore on_ready_callback if asked to execute a timer; reduce usage of void pointers * have the executor notify waitable fiddle with guard condition callbacks only if necessary Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

alsora requested review from ivanpauno, hidmic and wjwwood as code owners April 4, 2023 07:28

alsora mentioned this pull request Apr 4, 2023

add events-executor and timers-manager in rclcpp #2140

Closed

alsora commented Apr 4, 2023

View reviewed changes

mjcarroll mentioned this pull request Apr 11, 2023

Improve Executor Performance ros2/ros2#1395

Open

5 tasks