Fix high CPU - Use executor to spin and stop node in tf_listener thread #119

bpwilcox · 2019-06-19T21:22:54Z

Previously, #114 resolved the issue of terminating the work in the thread spinning the node which lead to hanging and unclean shutdown when destructing the node. However, the current implementation runs in a tight loop using rclcpp::spin_some, which leads to very high and inefficient cpu usage even when there is no work to be processed.

This problem was resolved in ros-navigation/navigation2#866, by putting the node on an executor and spinning. As opposed to rclcpp::spin, this change allows the executor to manage the termination of the spin thread by calling executor->cancel in the destructor. This pattern seems to work well when needing to cleanly join a thread spinning a node.

rotu · 2019-07-10T19:45:42Z

Would love to get this moving. TF_ROS is making robot_localization se_node peg one CPU core at 100% versus 30% with this PR.

@clalancette @cottsay

mjeronimo · 2019-07-15T17:43:24Z

@cottsay Would appreciate any attention you can provide here. This is also impacting the nav2 stack.

cottsay · 2019-07-15T20:39:05Z

tf2_ros/src/transform_listener.cpp

@@ -54,19 +54,24 @@ TransformListener::TransformListener(tf2::BufferCore & buffer, bool spin_thread)

 TransformListener::~TransformListener()
 {
-  stop_thread_ = true;
+  if (spin_thread_) {


Is this boolean actually necessary? Could we just use the unique_ptr's value and eliminate the extra value?

Suggested change

if (spin_thread_) {

if (executor_) {

Agreed. Pushed a commit to remove the extra boolean.

initialize executor set spin_thread to true in initThread Signed-off-by: bpwilcox <bpwilcox@eng.ucsd.edu>

cottsay

This change looks great to me, but it does break ABI, which makes me hesitant to nominate it for backport into Dashing. If you're OK with this landing in Eloquent, we can merge as-is, or we can iterate to find an ABI-compatible solution.

bpwilcox · 2019-07-18T17:22:40Z

I believe we'd like this for a Dashing backport. This change is internal to the transform listener interface, can you elaborate on how this breaks?

sloretz · 2019-07-18T17:29:32Z

This change is internal to the transform listener interface, can you elaborate on how this breaks?

@bpwilcox It looks like this patch could change the size of TransformListener if std::atomic<bool> and std::unique_ptr<...> are different sizes or have different alignment requirements.

cottsay · 2019-07-18T17:44:20Z

What if we keep the executor instance scoped within the thread, restore the bool, and use spin_some(std::chrono::nanoseconds) on the executor with some reasonable timeout before checking the bool for termination?

https://github.com/ros2/rclcpp/blob/8c48dbede7de2bc7e5199f340b6bfd33ffd6a784/rclcpp/include/rclcpp/executor.hpp#L192-L205

bpwilcox · 2019-07-19T22:11:55Z

What if we keep the executor instance scoped within the thread, restore the bool, and use spin_some(std::chrono::nanoseconds) on the executor with some reasonable timeout before checking the bool for termination?

This could be an interim solution (though I don't like throttling the spin with an arbitrary timeout), but I'd want this PR as-is (or perhaps using the alternative ros2/rclcpp#764 if merged) to go into the next release since it is more efficient.

I can try to test your suggestion locally and push a new PR for the Dashing backport. Should that also go to the ros2 branch or another branch?

cottsay · 2019-07-19T23:19:57Z

I opened #131. It looks like the duration values passed to spin_some and spin_once are different, and only the spin_once value acts as we would need it to, so I used that. So my earlier reference to spin_some are not correct.

clalancette · 2019-07-23T19:06:18Z

tf2_ros/include/tf2_ros/transform_listener.h

@@ -116,7 +116,7 @@ class TransformListener
  rclcpp::Subscription<tf2_msgs::msg::TFMessage>::SharedPtr message_subscription_tf_;
  rclcpp::Subscription<tf2_msgs::msg::TFMessage>::SharedPtr message_subscription_tf_static_;
  tf2::BufferCore & buffer_;
-  std::atomic<bool> stop_thread_{false};
+  std::unique_ptr<rclcpp::executors::SingleThreadedExecutor> executor_;


Looking at #133, I actually think that we can get rid of both the stop_thread_ and executor_ member variables, and just use a local executor in initThread.

If we want to use the local executor, should we go with the approach in #133 to be pushed to ros2, just removing the stop_thread?

More-or-less. For this PR targeting master, we don't care about ABI breaks, so we can remove both the stop_thread_ and executor_ member variables, and just use a local executor. For #133 targeting dashing, we'll still use the local executor, but we'll leave the stop_thread_ and executor_ member variables. They won't be used, but they'll maintain ABI. Make sense?

tf2_ros/src/transform_listener.cpp

remove executor member variable

bpwilcox · 2019-07-24T18:28:50Z

@clalancette I pushed the change to match #133 capturing the local executor so we can get this moving.

Also, if we believe this pattern to solve a common hangup, I'd suggest allowing for an executor to be passed into the rclcpp::spin function as I introduced in ros2/rclcpp#764

Reduce high CPU usage in TransformListener, and do it in a way that preserves ABI compatibility. This is a backport of #119. 1. Use the executor `spin` method, so the executor can idle when it has no work. 2. Capture the shared executor so we can (synchronously) call `cancel` in the `unique_ptr<thread>` deleter instead of polling `stop_thread_` in the thread run.

bpwilcox mentioned this pull request Jun 19, 2019

Add rclcpp::spin() version with executor reference ros2/rclcpp#764

Closed

bpwilcox changed the title ~~Use executor to spin and stop node in thread~~ Fix high CPU - Use executor to spin and stop node in tf_listener thread Jun 25, 2019

nuclearsandwich assigned cottsay Jul 11, 2019

cottsay reviewed Jul 15, 2019

View reviewed changes

bpwilcox added 2 commits July 17, 2019 11:29

use executor to spin and stop node in thread

bef6170

initialize executor set spin_thread to true in initThread Signed-off-by: bpwilcox <bpwilcox@eng.ucsd.edu>

remove spin_thread_ bool

249fe87

bpwilcox force-pushed the tf_listener_executor_thread branch from aabbd6d to 249fe87 Compare July 17, 2019 18:39

cottsay approved these changes Jul 18, 2019

View reviewed changes

cottsay added the bug Something isn't working label Jul 18, 2019

cottsay mentioned this pull request Jul 19, 2019

Fix for high CPU usage in transform_listener #131

Closed

rotu mentioned this pull request Jul 23, 2019

[Dashing backport] Fix high CPU - Use executor to spin and stop node in tf_listener thread #133

Merged

clalancette reviewed Jul 23, 2019

View reviewed changes

capture local executor in lambda

74ee27b

remove executor member variable

bpwilcox force-pushed the tf_listener_executor_thread branch from 4160862 to 74ee27b Compare July 24, 2019 18:22

cottsay approved these changes Jul 29, 2019

View reviewed changes

clalancette approved these changes Jul 30, 2019

View reviewed changes

cottsay merged commit fccfd98 into ros2:ros2 Jul 31, 2019

crdelsey mentioned this pull request Aug 8, 2019

High CPU usage even at idle ros-navigation/navigation2#1015

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix high CPU - Use executor to spin and stop node in tf_listener thread #119

Fix high CPU - Use executor to spin and stop node in tf_listener thread #119

bpwilcox commented Jun 19, 2019

rotu commented Jul 10, 2019

mjeronimo commented Jul 15, 2019

cottsay Jul 15, 2019

bpwilcox Jul 17, 2019

cottsay left a comment

bpwilcox commented Jul 18, 2019

sloretz commented Jul 18, 2019

cottsay commented Jul 18, 2019

bpwilcox commented Jul 19, 2019

cottsay commented Jul 19, 2019

clalancette Jul 23, 2019

bpwilcox Jul 24, 2019

clalancette Jul 24, 2019

bpwilcox commented Jul 24, 2019

Fix high CPU - Use executor to spin and stop node in tf_listener thread #119

Fix high CPU - Use executor to spin and stop node in tf_listener thread #119

Conversation

bpwilcox commented Jun 19, 2019

rotu commented Jul 10, 2019

mjeronimo commented Jul 15, 2019

cottsay Jul 15, 2019

Choose a reason for hiding this comment

bpwilcox Jul 17, 2019

Choose a reason for hiding this comment

cottsay left a comment

Choose a reason for hiding this comment

bpwilcox commented Jul 18, 2019

sloretz commented Jul 18, 2019

cottsay commented Jul 18, 2019

bpwilcox commented Jul 19, 2019

cottsay commented Jul 19, 2019

clalancette Jul 23, 2019

Choose a reason for hiding this comment

bpwilcox Jul 24, 2019

Choose a reason for hiding this comment

clalancette Jul 24, 2019

Choose a reason for hiding this comment

bpwilcox commented Jul 24, 2019