Be more proactive about skipping work if possible #685

mjcarroll · 2023-04-07T20:16:14Z

This makes rmw_wait more aggressive about how it skips work.

In the case that a condition is already met, this will skip attaching/detaching conditions from the underlying waitset and skip waiting.

In the case that a wait is required, it generally looks like this:

and when a wait isn't needed:

mjcarroll · 2023-04-07T20:19:48Z

Should skip wait has a relatively constant cost:

And postprocess

clalancette · 2023-04-10T13:38:20Z

rmw_fastrtps_shared_cpp/CMakeLists.txt

@@ -49,6 +49,9 @@ find_package(fastcdr REQUIRED CONFIG)
 find_package(fastrtps 2.3 REQUIRED CONFIG)
 find_package(FastRTPS 2.3 REQUIRED MODULE)

+find_package(Threads REQUIRED)
+find_package(tracy_vendor REQUIRED)


What's with the tracy_vendor here? Do we need this to get this work in?

No, there are some profiling points in this branch, I will remove for final merge if we determine this is a valid approach.

rmw_fastrtps_shared_cpp/src/rmw_wait.cpp

mjcarroll · 2023-04-10T14:15:06Z

@MiguelCompany I am also looking at adding a method to the waitset to allow attaching/detaching multiple conditions. I think this can save a bit of churn in locking/unlocking mutexes: https://github.com/mjcarroll/Fast-DDS/tree/mjcarroll/attach_multiple_conditions

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Signed-off-by: Michael Carroll <michael@openrobotics.org> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>

Signed-off-by: Michael Carroll <michael@openrobotics.org>

mjcarroll · 2023-04-10T18:30:42Z

@clalancette this one is ready for you.

mjcarroll · 2023-04-10T18:32:54Z

Linux
Linux-aarch64
Windows

clalancette

I've left just a few nits inline. All of these are small enough that a "it compiles for me" comment is good enough so we don't have to restart CI.

I went through this with @mjcarroll in some detail. In short, what this does is to change it so that we no longer call wait_set->wait() if there is already work waiting for us. If that is the case, then we also don't need to attach/detach conditions to the wait_set if we already have work, and we can just go through and null out the entries that don't have work waiting. If we do have work waiting for us, then we need to do the whole dance.

I will note that this is much faster in the best case, where the first guard condition we are waiting on is triggered. In that case, we'll do much less work than before. However, if there is no work for us to do, then this is actually somewhat slower than before since we need to iterate over the lists 3 times instead of 2. I think this is an acceptable tradeoff for now, but it bears looking into further to see if we can reduce how expensive get_first_untaken_info is.

Anyway, with my nits fixed, I'm happy to approve this. It looks like a net win for now.

clalancette · 2023-04-10T19:28:42Z

rmw_fastrtps_shared_cpp/src/rmw_wait.cpp

-  if (!skip_wait) {
-    timeout = (wait_timeout) ?
+    // Attach all of the conditions to the wait set.
+    // \TODO(mjcarroll) When upstream has the ability to attach a vector of conditions,


Suggested change

// \TODO(mjcarroll) When upstream has the ability to attach a vector of conditions,

// TODO(mjcarroll): When upstream has the ability to attach a vector of conditions,

clalancette · 2023-04-10T19:28:55Z

rmw_fastrtps_shared_cpp/src/rmw_wait.cpp

+    wait_result = (ret_code == ReturnCode_t::RETCODE_OK);
+
+    // Detach all of the conditions from the wait set.
+    // \TODO(mjcarroll) When upstream has the ability to detach a vector of conditions,


Suggested change

// \TODO(mjcarroll) When upstream has the ability to detach a vector of conditions,

// TODO(mjcarroll): When upstream has the ability to detach a vector of conditions,

clalancette · 2023-04-10T19:41:21Z

rmw_fastrtps_shared_cpp/src/rmw_wait.cpp

+ * \param[in] events events to check
+ * \return true if any condition is triggered, false otherwise
+ */
+bool has_triggered_condition(


Suggested change

bool has_triggered_condition(

static bool has_triggered_condition(

(or an anonymous namespace, I really don't have a preference)

clalancette · 2023-04-10T20:29:33Z

rmw_fastrtps_shared_cpp/src/rmw_wait.cpp

+      auto custom_subscriber_info = static_cast<CustomSubscriberInfo *>(data);
+      eprosima::fastdds::dds::SampleInfo sample_info;
+      if (ReturnCode_t::RETCODE_OK ==
+        custom_subscriber_info->data_reader_->get_first_untaken_info(&sample_info))


I think it is worthwhile to add a comment saying that this is relatively slow, so it is worth skipping it if possible.

mjcarroll · 2023-04-10T20:57:58Z

I will note that this is much faster in the best case, where the first guard condition we are waiting on is triggered. In that case, we'll do much less work than before. However, if there is no work for us to do, then this is actually somewhat slower than before since we need to iterate over the lists 3 times instead of 2. I think this is an acceptable tradeoff for now, but it bears looking into further to see if we can reduce how expensive get_first_untaken_info is.

Based on my testing with https://github.com/irobot-ros/ros2-performance, there is less tradeoff than you would think at first. We do add a second set of iterations before the wait call, but in the case that we are actually waiting, that time would have been yielded to waiting on the waitset anyway. The amount of work after the wait is over remains the same, so the event-to-callback latency is the same.

In the case that there is something already ready, we skip attaching the conditions, which are somewhat expensive (each requires a lock and unlock of a mutex) as well as skipping the get_first_untaken_info which is also expensive.

mjcarroll · 2023-04-10T21:02:52Z

Two potential enhancements here (but out of the scope of this PR):

Add a FastDDS waitset api that allows for the addition and removal of many conditions. Since each time a condition is inserted, the waitset api iterates the list of all conditions to check for the existence already, this can add up in the case of many conditions. I'm not exactly sure what we would want the api to look like, but I have protoyped remove_all_conditions and add_conditions on a branch to good results. The add_conditions that takes a sequence clears the waitset so that we don't have to check for already inserted as well. Additionally, this can be done under a single lock, rather than having to re-take the lock every iteration.
Add/find a waitset api that is cheaper to check if data is available. In this case, I think we only need a boolean to indicate if there is data in need of servicing, rather than populating the SampleInfo structure that we aren't doing anything else. Maybe @MiguelCompany has a better idea of an API to use here.

Signed-off-by: Michael Carroll <michael@openrobotics.org>

mjcarroll · 2023-04-10T21:17:07Z

All of these are small enough that a "it compiles for me" comment is good enough

It compiles for me. But for good measure, I'm throwing in a RHEL and Windows Debug build:

RHEL:
Windbg:

mjcarroll · 2023-04-11T02:02:13Z

Alright, everything but Windows debug is green, so I'm calling this good.

In the case that a condition is already triggered, skip attaching/detaching conditions as well as the actual wait call. This reduces call time in many cases where work is already queued. Signed-off-by: Michael Carroll <michael@openrobotics.org> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>

mjcarroll requested review from gbiggs and sloretz as code owners April 7, 2023 20:16

clalancette reviewed Apr 10, 2023

View reviewed changes

MiguelCompany suggested changes Apr 10, 2023

View reviewed changes

rmw_fastrtps_shared_cpp/src/rmw_wait.cpp Outdated Show resolved Hide resolved

Be more proactive about skipping work if possible

ea002ca

Signed-off-by: Michael Carroll <michael@openrobotics.org>

mjcarroll force-pushed the mjcarroll/faster_rmw_wait branch from 388712d to d8d95de Compare April 10, 2023 18:21

mjcarroll and others added 2 commits April 10, 2023 13:26

Update rmw_fastrtps_shared_cpp/src/rmw_wait.cpp

4dde469

Signed-off-by: Michael Carroll <michael@openrobotics.org> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>

Remove tracy from cmakelists and remove profile points

ce5f9a2

Signed-off-by: Michael Carroll <michael@openrobotics.org>

mjcarroll force-pushed the mjcarroll/faster_rmw_wait branch from d8d95de to ce5f9a2 Compare April 10, 2023 18:29

clalancette requested changes Apr 10, 2023

View reviewed changes

Address reviewer feedback

7cd2cd5

Signed-off-by: Michael Carroll <michael@openrobotics.org>

clalancette approved these changes Apr 10, 2023

View reviewed changes

mjcarroll merged commit d93f410 into rolling Apr 11, 2023

delete-merged-branch bot deleted the mjcarroll/faster_rmw_wait branch April 11, 2023 02:05

mjcarroll mentioned this pull request Apr 11, 2023

Improve Executor Performance ros2/ros2#1395

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Be more proactive about skipping work if possible #685

Be more proactive about skipping work if possible #685

mjcarroll commented Apr 7, 2023 •

edited

Loading

mjcarroll commented Apr 7, 2023

clalancette Apr 10, 2023

mjcarroll Apr 10, 2023

mjcarroll commented Apr 10, 2023

mjcarroll commented Apr 10, 2023

mjcarroll commented Apr 10, 2023

clalancette left a comment

clalancette Apr 10, 2023

clalancette Apr 10, 2023

clalancette Apr 10, 2023

clalancette Apr 10, 2023

mjcarroll commented Apr 10, 2023

mjcarroll commented Apr 10, 2023

mjcarroll commented Apr 10, 2023 •

edited by clalancette

Loading

mjcarroll commented Apr 11, 2023

	// \TODO(mjcarroll) When upstream has the ability to attach a vector of conditions,
	// TODO(mjcarroll): When upstream has the ability to attach a vector of conditions,

	bool has_triggered_condition(
	static bool has_triggered_condition(

Be more proactive about skipping work if possible #685

Be more proactive about skipping work if possible #685

Conversation

mjcarroll commented Apr 7, 2023 • edited Loading

mjcarroll commented Apr 7, 2023

clalancette Apr 10, 2023

Choose a reason for hiding this comment

mjcarroll Apr 10, 2023

Choose a reason for hiding this comment

mjcarroll commented Apr 10, 2023

mjcarroll commented Apr 10, 2023

mjcarroll commented Apr 10, 2023

clalancette left a comment

Choose a reason for hiding this comment

clalancette Apr 10, 2023

Choose a reason for hiding this comment

clalancette Apr 10, 2023

Choose a reason for hiding this comment

clalancette Apr 10, 2023

Choose a reason for hiding this comment

clalancette Apr 10, 2023

Choose a reason for hiding this comment

mjcarroll commented Apr 10, 2023

mjcarroll commented Apr 10, 2023

mjcarroll commented Apr 10, 2023 • edited by clalancette Loading

mjcarroll commented Apr 11, 2023

mjcarroll commented Apr 7, 2023 •

edited

Loading

mjcarroll commented Apr 10, 2023 •

edited by clalancette

Loading