Thanks for the information!
I've reflected update #1628.

did you test locally if this solve #1487 and #1008?
I'm afraid that the extra global mutex can affect the behavior.

I see. I didn't test it enough.
I'll check the behavior of the two issues.

Sure, there is a risk because the variables are not encapsulated.
I'd like to focus on performance though, is there any better implementation?

Or, how about changing them from member variables to static global variables in multi_threaded_executor.cpp?

// multi_threaded_executor.cpp
static std::unordered_map<MultiThreadedExecutor *, std::shared_ptr<MutexTwoPriorities>> wait_mutex_set_;
static std::mutex MultiThreadedExecutor::shared_wait_mutex_;

At any rate, the test results look fine.

I compared single-threaded and multi-threaded executor, ref: #1487 (comment).
source code : https://github.com/hsgwa/executor_comparison.git

Comparison: Multi-threaded Executor

publish rate

%CPU

The following is a comparison of the %cpu columns of pidstat.

Comparison: Single-threaded Executor

%CPU

Comparison: timer_over_take test

I compared the diff to be evaluated, ref : #1008.

diff --git a/rclcpp/test/rclcpp/executors/test_multi_threaded_executor.cpp b/rclcpp/test/rclcpp/executors/test_multi_threaded_executor.cpp
index b6fd2c3f..ca27da2c 100644
--- a/rclcpp/test/rclcpp/executors/test_multi_threaded_executor.cpp
+++ b/rclcpp/test/rclcpp/executors/test_multi_threaded_executor.cpp
@@ -17,6 +17,7 @@
#include <chrono>
#include <string>
#include <memory>
+#include <iostream>

#include "rclcpp/exceptions.hpp"
#include "rclcpp/node.hpp"
@@ -90,6 +91,7 @@ TEST_F(TestMultiThreadedExecutor, timer_over_take) {
          executor.cancel();
          ASSERT_GT(diff, PERIOD - TOLERANCE);
        }
+       std::cout << diff << std::endl;
      }
    };

I compared the diff to be evaluated, ref : #1008.

Instead of that, could you repeat the test to see if it's flaky or not?

stress --cpu 8 &  # use number of cores instead of 8
for i in {1..100}; do ./test_multi_threaded_executor; if [ $? -ne 0 ]; then break;fi;clear; done

Thanks!

I tried to test it in a loop 100 times, but it failed in the middle.

$ stress --cpu 8 &  # use number of cores
$ for i in {1..100}; do ./test_multi_threaded_executor; if [ $? -ne 0 ]; then break;fi;clear; done

To check the effect before and after the modification, I executed the following commands 10 times each and counted the number of consecutive successful tests.

$ stress --cpu 8 &  # use number of cores
$ for i in {1..100}; do ./test_multi_threaded_executor; if [ $? -ne 0 ]; then break;fi; done | grep "PASSED" --line-buffered | wc -l

Both failed in the middle of the test, so there seems to be no obvious difference.

@jacobperron friendly ping. could you give me a review if you don't mind?

CI:

• Linux
• Linux-aarch64
• macOS
• Windows