API Changes for Multihtreaded Events Executor #2466

jmachowinski · 2024-03-25T15:19:51Z

These are the API changes needed in order to implement an external Multihtreaded Events Executor

@mjcarroll @alsora @fujitatomoya

alsora

Looks good to me.
I would suggest to add unit-tests for the Clock object

jmachowinski · 2024-03-25T17:47:03Z

Writing tests was a good suggestion, I screwed up the clock interface, it's not thread safe, I will redesign it...

rclcpp/test/benchmark/benchmark_executor.cpp

jmachowinski · 2024-03-26T14:23:50Z

Redesigned the clock sleep interruption. Can the reviewers have a good lock at the mutex/lock/wait construct, its hard to get it right without data races....

rclcpp/src/rclcpp/clock.cpp

jmachowinski · 2024-04-05T15:59:50Z

I'll need to add a way to terminate the clock and all sleeps as soon as the shutdown call was done.

fujitatomoya

overall lgtm, a couple of minor comments.

rclcpp/include/rclcpp/clock.hpp

rclcpp/test/rclcpp/test_clock.cpp

jmachowinski · 2024-04-08T15:43:45Z

rebased to rolling

mjcarroll · 2024-04-09T00:08:42Z

Waiting for this to get in before running CI: ros2/rosidl_typesupport_fastrtps#117

mjcarroll · 2024-04-10T00:24:33Z

Linux
Linux-aarch64
Windows

jmachowinski · 2024-04-10T11:58:40Z

rebased and (hopefully) fixed windows compile error

alsora · 2024-04-10T14:52:16Z

Linux
Linux-aarch64
Windows

jmachowinski · 2024-04-10T18:26:41Z

Dropped the test refactoring.

mjcarroll · 2024-04-12T13:29:31Z

Linux
Linux-aarch64
Windows

mjcarroll · 2024-04-12T14:08:45Z

@jmachowinski this needs a merge from rolling because of the added API

jmachowinski · 2024-04-12T14:31:02Z

rebased. test_executors_timer_cancel_behavior is flaky on my machine. I hope this gets fixed by #2500

mjcarroll · 2024-04-12T15:04:08Z

Linux
Linux-aarch64
Windows

fujitatomoya

lgtm with green CI

wjwwood · 2024-04-12T23:34:28Z

I'm highly suspect of the changes to the Executor API to make it so you don't need to inherit from rclcpp::Executor. I don't think we should go this route until it's explained better why that's necessary.

jmachowinski · 2024-04-13T13:10:25Z

rebased to rolling

This is useful in case a second thread needs to wake up another thread, that is sleeping using a clock. Signed-off-by: Janosch Machowinski <[email protected]>

This adds support for multiple threads waiting on the same clock, while an shutdown is invoked. Signed-off-by: Janosch Machowinski <[email protected]>

jmachowinski · 2024-04-14T12:37:31Z

I reworked this PR. Now everything in rclcpp::Executor is made virtual.

I changed two function, that were template function, without any reason to normal functions.

I reworked the template function spin_until_future_complete, so that it can be fully overwritten by a subclass.

I added an constructor, that will not touch anything in the system.

@wjwwood can you do a review ?

This commit makes every public funciton virtual, and adds virtual impl function for the existing template functions. The goal of this commit is to be able to fully control the everything from a derived class. Signed-off-by: Janosch Machowinski <[email protected]>

wjwwood

Pretty close, had a few small style things and a suggestion to scope down the changes.

Also, I think there might still be some style issues? please run ament_uncrustify path/to/rclcpp --reformat if you haven't already (saw some if( that should be if ().

wjwwood · 2024-04-15T12:07:25Z

rclcpp/include/rclcpp/executor.hpp

-  template<typename RepT = int64_t, typename T = std::milli>
-  void
+  RCLCPP_PUBLIC
+  virtual void
  spin_node_once(
-    rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node,
-    std::chrono::duration<RepT, T> timeout = std::chrono::duration<RepT, T>(-1))
-  {
-    return spin_node_once_nanoseconds(
-      node,
-      std::chrono::duration_cast<std::chrono::nanoseconds>(timeout)
-    );
-  }
+    const rclcpp::node_interfaces::NodeBaseInterface::SharedPtr & node,
+    std::chrono::nanoseconds timeout = std::chrono::nanoseconds(-1));

  /// Convenience function which takes Node and forwards NodeBaseInterface.
-  template<typename NodeT = rclcpp::Node, typename RepT = int64_t, typename T = std::milli>
-  void
+  RCLCPP_PUBLIC
+  virtual void
  spin_node_once(
-    std::shared_ptr<NodeT> node,
-    std::chrono::duration<RepT, T> timeout = std::chrono::duration<RepT, T>(-1))
-  {
-    return spin_node_once_nanoseconds(
-      node->get_node_base_interface(),
-      std::chrono::duration_cast<std::chrono::nanoseconds>(timeout)
-    );
-  }
+    const std::shared_ptr<rclcpp::Node> & node,
+    std::chrono::nanoseconds timeout = std::chrono::nanoseconds(-1));


Do you absolutely need to make these non-template? I'm afraid existing code may not compile against this if certain types of durations are passed.

I don't really need to, but all other functions in the executor have the same signature and compile just fine. Any std::chono::duration type should auto convert to std::chrono::nanoseconds.

Arguably the others taking only nanoseconds is a probably a bug, see: https://stackoverflow.com/a/22362867

It seems like in C++20 we could have a function that takes std::chrono::duration<auto>, but we don't have that right now.

I'll remove this change in my fix up.

rclcpp/include/rclcpp/executor.hpp

wjwwood · 2024-04-15T12:20:53Z

rclcpp/include/rclcpp/executor.hpp

@@ -447,6 +403,10 @@ class Executor
    rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node,
    std::chrono::nanoseconds timeout);

+  virtual FutureReturnCode spin_until_future_complete_impl(


Suggested change

virtual FutureReturnCode spin_until_future_complete_impl(

RCLCPP_PUBLIC

virtual FutureReturnCode

spin_until_future_complete_impl(

Also, needs at least a minimal docstring.

As I said above, I'd consider dropping this change, but if you keep it, consider the name spin_until_future_complete_nanoseconds() since that would match other functions like spin_node_once() -> spin_node_once_nanoseconds().

We got two colliding styles here, there is also
spin_once -> spin_once_impl and spin_some -> spin_some_impl

We got two colliding styles here, there is also
spin_once -> spin_once_impl and spin_some -> spin_some_impl

Yeah, I'll leave it as-is, but I think this is related to the templated functions that take duration vs just taking nanoseconds, I still believe the templated approach is better, and in that case the narrowing to the non-template protected methods are truly just to cast it down to nanoseconds, so that makes sense in those cases. Likely if I had noticed I would have suggesting the newer functions follow that pattern, but it's not that important now.

wjwwood · 2024-04-15T12:22:07Z

rclcpp/include/rclcpp/executor.hpp

-    // TODO(wjwwood): does not work recursively; can't call spin_node_until_future_complete
-    // inside a callback executed by an executor.
-
-    // Check the future before entering the while loop.
-    // If the future is already complete, don't try to spin.
-    std::future_status status = future.wait_for(std::chrono::seconds(0));
-    if (status == std::future_status::ready) {
-      return FutureReturnCode::SUCCESS;
-    }
-
-    auto end_time = std::chrono::steady_clock::now();
-    std::chrono::nanoseconds timeout_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(
-      timeout);
-    if (timeout_ns > std::chrono::nanoseconds::zero()) {
-      end_time += timeout_ns;
-    }
-    std::chrono::nanoseconds timeout_left = timeout_ns;
-
-    if (spinning.exchange(true)) {
-      throw std::runtime_error("spin_until_future_complete() called while already spinning");
+    return spin_until_future_complete_impl(std::chrono::duration_cast<std::chrono::nanoseconds>(
+      timeout), [&future] () {
+               return future.wait_for(std::chrono::seconds(0));
    }
-    RCPPUTILS_SCOPE_EXIT(this->spinning.store(false); );
-    while (rclcpp::ok(this->context_) && spinning.load()) {
-      // Do one item of work.
-      spin_once_impl(timeout_left);
-
-      // Check if the future is set, return SUCCESS if it is.
-      status = future.wait_for(std::chrono::seconds(0));
-      if (status == std::future_status::ready) {
-        return FutureReturnCode::SUCCESS;
-      }
-      // If the original timeout is < 0, then this is blocking, never TIMEOUT.
-      if (timeout_ns < std::chrono::nanoseconds::zero()) {
-        continue;
-      }
-      // Otherwise check if we still have time to wait, return TIMEOUT if not.
-      auto now = std::chrono::steady_clock::now();
-      if (now >= end_time) {
-        return FutureReturnCode::TIMEOUT;
-      }
-      // Subtract the elapsed time from the original timeout.
-      timeout_left = std::chrono::duration_cast<std::chrono::nanoseconds>(end_time - now);
-    }
-
-    // The future did not complete before ok() returned false, return INTERRUPTED.
-    return FutureReturnCode::INTERRUPTED;
+    );


I'd recommend dropping this change for now. It's likely to collide with #2475 if we merge that one and I don't think it's absolutely necessary (smaller changes are easier to get in when there's a lot of activity).

I would like to keep this change, as it allows me to use a second thread to wait for the future, instead of the current construct. But if you have a hard opinion on dropping it, I am open for it.

wjwwood · 2024-04-15T12:26:14Z

rclcpp/src/rclcpp/executor.cpp

+Executor::Executor(const std::shared_ptr<rclcpp::Context> & context)
+: spinning(false),
+  entities_need_rebuild_(true),
+  collector_(nullptr),
+  wait_set_({}, {}, {}, {}, {}, {}, context)
+{
+}
+


This is better (as a workaround), it will be easier (in my opinion) to deprecate when we implement a base class after the release, as opposed to re-restricting the functions that currently require the executor base class.

rclcpp/include/rclcpp/executor.hpp

wjwwood · 2024-04-15T12:37:45Z

rclcpp/test/rclcpp/test_clock.cpp

+    // make sure the thread is already sleeping before we send the cancel
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));


This is kind of a weak assumption (that the thread will start within 100ms), it would be better if we verified the thread had started before continuing, though there would still be a race between setting a bool in the thread and the next line of the thread that does sleep_until() is called. Ideally we'd be able to see when the clock is sleeping somehow. But I don't think that's possible. Not sure anything should change, just wanted to point it out.

Co-authored-by: William Woodall <[email protected]> Signed-off-by: jmachowinski <[email protected]>

jmachowinski · 2024-04-15T14:01:24Z

Pretty close, had a few small style things and a suggestion to scope down the changes.

Also, I think there might still be some style issues? please run ament_uncrustify path/to/rclcpp --reformat if you haven't already (saw some if( that should be if ().

I did that, uncrustify doesn't fix this (?). BTW, why are we not using clang-format ? In my experience it works way better then the uncrustify / cpplint construct.

Signed-off-by: Janosch Machowinski <[email protected]>

This change allows it to use a second thread to wait for the future to become ready. Signed-off-by: Janosch Machowinski <[email protected]>

wjwwood · 2024-04-16T02:45:18Z

I did that, uncrustify doesn't fix this (?).

I did this and if found the issues (and auto-formatted the fix when I used --reformat):

william@markook-ubuntu-2204:~/ros2_ws/src/ros2/rclcpp (git:wjwwood/executor_features:163f727)
$ ament_uncrustify .
...
Code style divergence in file 'rclcpp/test/rclcpp/test_clock.cpp':

--- rclcpp/test/rclcpp/test_clock.cpp
+++ rclcpp/test/rclcpp/test_clock.cpp.uncrustify
@@ -116,4 +116,4 @@
-        Clocks,
-        TestClockWakeup,
-        ::testing::Values(
-            RCL_SYSTEM_TIME, RCL_ROS_TIME, RCL_STEADY_TIME
+  Clocks,
+  TestClockWakeup,
+  ::testing::Values(
+    RCL_SYSTEM_TIME, RCL_ROS_TIME, RCL_STEADY_TIME
@@ -185,2 +185,3 @@
-  for(size_t nr = 0; nr < thread_finished.size(); nr++) {
-    threads.push_back(std::thread(
+  for (size_t nr = 0; nr < thread_finished.size(); nr++) {
+    threads.push_back(
+      std::thread(
@@ -189 +190 @@
-        // make sure the thread starts sleeping late
+          // make sure the thread starts sleeping late
@@ -192 +193 @@
-      }));
+        }));
@@ -198 +199 @@
-  for(const bool & finished : thread_finished) {
+  for (const bool & finished : thread_finished) {
@@ -209,2 +210,2 @@
-    for(const bool finished : thread_finished) {
-      if(!finished) {
+    for (const bool finished : thread_finished) {
+      if (!finished) {
@@ -219 +220 @@
-  for(const bool finished : thread_finished) {
+  for (const bool finished : thread_finished) {
@@ -223 +224 @@
-  for(auto & thread : threads) {
+  for (auto & thread : threads) {
...

Are you running it correctly?

clalancette · 2024-04-16T02:53:52Z

BTW, why are we not using clang-format ? In my experience it works way better then the uncrustify / cpplint construct.

We've had many long, detailed conversations about this, some as recently as a month ago. While I won't rehash all of the arguments, the short of it is:

It is not possible to configure clang-format to meet our preferred style.
Even if we were to change our preferred style to meet what clang-format wants, we have literally hundreds of packages in the core that would need to be changed. And they would all have to be changed at once. And it would mean that we could no longer cleanly backport from Rolling to stable branches.

We have many real problems in the code, this doesn't seem to be one of them (in my opinion).

wjwwood · 2024-04-16T03:37:07Z

Closing in favor of #2506

wjwwood · 2024-04-16T03:59:19Z

better then the uncrustify / cpplint construct.

Also, for what it's worth, I think clang-tidy might overlap a bit with cpplint, but it wouldn't completely replace it. most projects that use cpplint also use clang-tidy or something similar along side it.

jmachowinski · 2024-04-16T08:10:12Z

Are you running it correctly?

Here is a paste of my system, it is clearly not working, but why ?

https://pastebin.com/gGbPLdaA

clalancette · 2024-04-16T23:13:27Z

Here is a paste of my system, it is clearly not working, but why ?

Oh, you know, we've recently updated to a new version of uncrustify (0.78.1), that does things slightly differently than the old way. What version of uncrustify are you using?

jmachowinski · 2024-04-17T09:41:24Z

Oh, you know, we've recently updated to a new version of uncrustify (0.78.1), that does things slightly differently than the old way. What version of uncrustify are you using?

Recent installation, uncrustify_vendor installs 0.78.1f

$ uncrustify --version
Uncrustify-0.78.1_f

I even patched ament_uncrustify to check if it picked up the right version, and this one also reports Uncrustify-0.78.1_f

I have an Uncrustify-0.72.0_f installed by apt, but even if I uninstall this version, my system still reports no style divergence.

jmachowinski requested review from ivanpauno, hidmic and wjwwood as code owners March 25, 2024 15:19

jmachowinski force-pushed the executor_features branch from 07eaf14 to 1404d45 Compare March 25, 2024 15:25

alsora reviewed Mar 25, 2024

View reviewed changes

jmachowinski marked this pull request as draft March 25, 2024 17:46

Zard-C reviewed Mar 25, 2024

View reviewed changes

rclcpp/test/benchmark/benchmark_executor.cpp Outdated Show resolved Hide resolved

jmachowinski force-pushed the executor_features branch from 1404d45 to 36061ed Compare March 26, 2024 14:19

jmachowinski marked this pull request as ready for review March 26, 2024 14:26

alsora reviewed Mar 26, 2024

View reviewed changes

rclcpp/src/rclcpp/clock.cpp Show resolved Hide resolved

quarkytale assigned mjcarroll Apr 4, 2024

fujitatomoya reviewed Apr 5, 2024

View reviewed changes

rclcpp/include/rclcpp/clock.hpp Outdated Show resolved Hide resolved

rclcpp/test/rclcpp/test_clock.cpp Outdated Show resolved Hide resolved

jmachowinski force-pushed the executor_features branch from c118463 to cb3e4cc Compare April 8, 2024 15:42

jmachowinski force-pushed the executor_features branch from cb3e4cc to 1795139 Compare April 10, 2024 11:58

jmachowinski force-pushed the executor_features branch from 1795139 to eafb691 Compare April 10, 2024 18:25

jmachowinski force-pushed the executor_features branch from eafb691 to 30e7304 Compare April 12, 2024 14:29

fujitatomoya approved these changes Apr 12, 2024

View reviewed changes

jmachowinski force-pushed the executor_features branch from 30e7304 to 76c970b Compare April 13, 2024 13:10

Janosch Machowinski and others added 2 commits April 14, 2024 14:32

feat(clock): Added function to interrupt sleep

5077974

This is useful in case a second thread needs to wake up another thread, that is sleeping using a clock. Signed-off-by: Janosch Machowinski <[email protected]>

feat: Added support for multi thread wait / shutdown

03e5f9a

This adds support for multiple threads waiting on the same clock, while an shutdown is invoked. Signed-off-by: Janosch Machowinski <[email protected]>

jmachowinski force-pushed the executor_features branch from 76c970b to d50b888 Compare April 14, 2024 12:33

jmachowinski force-pushed the executor_features branch from d50b888 to 34e2a80 Compare April 14, 2024 12:37

wjwwood mentioned this pull request Apr 15, 2024

Add support for spin_until_complete (take 2) #2475

Open

wjwwood reviewed Apr 15, 2024

View reviewed changes

jmachowinski and others added 4 commits April 15, 2024 15:47

Update rclcpp/include/rclcpp/executor.hpp

77799c9

Co-authored-by: William Woodall <[email protected]> Signed-off-by: jmachowinski <[email protected]>

Update rclcpp/include/rclcpp/executor.hpp

f85ad12

Co-authored-by: William Woodall <[email protected]> Signed-off-by: jmachowinski <[email protected]>

Update rclcpp/include/rclcpp/executor.hpp

c72e601

Co-authored-by: William Woodall <[email protected]> Signed-off-by: jmachowinski <[email protected]>

Update rclcpp/include/rclcpp/executor.hpp

2ebcb49

Co-authored-by: William Woodall <[email protected]> Signed-off-by: jmachowinski <[email protected]>

Janosch Machowinski added 2 commits April 15, 2024 16:30

docs: added doc string for spin_until_future_complete_impl

da85eb0

Signed-off-by: Janosch Machowinski <[email protected]>

made is_spinning not virtual

3b8c558

Signed-off-by: Janosch Machowinski <[email protected]>

jmachowinski force-pushed the executor_features branch from c341f21 to 7804051 Compare April 15, 2024 14:30

feat: Changed interface of spin_until_future_complete_impl

163f727

This change allows it to use a second thread to wait for the future to become ready. Signed-off-by: Janosch Machowinski <[email protected]>

jmachowinski force-pushed the executor_features branch from 7804051 to 163f727 Compare April 15, 2024 14:34

wjwwood mentioned this pull request Apr 16, 2024

Reduce overhead for inheriting from rclcpp::Executor when base functionality is not reused #2506

Merged

wjwwood closed this Apr 16, 2024

		// make sure the thread is already sleeping before we send the cancel
		std::this_thread::sleep_for(std::chrono::milliseconds(100));

API Changes for Multihtreaded Events Executor #2466

API Changes for Multihtreaded Events Executor #2466

Conversation

jmachowinski commented Mar 25, 2024

alsora left a comment

Choose a reason for hiding this comment

jmachowinski commented Mar 25, 2024

jmachowinski commented Mar 26, 2024

jmachowinski commented Apr 5, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

jmachowinski commented Apr 8, 2024

mjcarroll commented Apr 9, 2024

mjcarroll commented Apr 10, 2024

jmachowinski commented Apr 10, 2024

alsora commented Apr 10, 2024

jmachowinski commented Apr 10, 2024

mjcarroll commented Apr 12, 2024

mjcarroll commented Apr 12, 2024

jmachowinski commented Apr 12, 2024

mjcarroll commented Apr 12, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

wjwwood commented Apr 12, 2024

jmachowinski commented Apr 13, 2024

jmachowinski commented Apr 14, 2024 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmachowinski commented Apr 15, 2024 • edited Loading

wjwwood commented Apr 16, 2024

clalancette commented Apr 16, 2024

wjwwood commented Apr 16, 2024

wjwwood commented Apr 16, 2024

jmachowinski commented Apr 16, 2024

clalancette commented Apr 16, 2024

jmachowinski commented Apr 17, 2024

jmachowinski commented Apr 14, 2024 •

edited

Loading

jmachowinski commented Apr 15, 2024 •

edited

Loading