Avoid unecessary creation of MultiThreadedExecutor#3090
Avoid unecessary creation of MultiThreadedExecutor#3090ahcorde merged 2 commits intoros2:rollingfrom
Conversation
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
| auto exec = std::make_shared<rclcpp::executors::MultiThreadedExecutor>(); | ||
| auto node = std::make_shared<rclcpp_components::ComponentManager>(); | ||
| rclcpp::executors::MultiThreadedExecutor::SharedPtr exec; | ||
| const auto node = std::make_shared<rclcpp_components::ComponentManager>(); |
There was a problem hiding this comment.
I don't believe that we can mark this as const, because the set_executor function mutates the node.
Did this compile for you?
There was a problem hiding this comment.
I didn't outright compile it, but my IDE (CLion) which uses clangd for its intentions said it was fine, which means that it should compile
the const applies to the shared_ptr, not the Node itself. const Node::SharedPtr (const shared_ptr<Node>) is different from Node::ConstSharedPtr (shared_ptr<const Node>)
you're thinking of the second one, but this is the first
| node->set_executor(exec); | ||
| } else { | ||
| node->set_executor(exec); | ||
| exec = std::make_shared<rclcpp::executors::MultiThreadedExecutor>(); |
There was a problem hiding this comment.
We could also consider using parameter_or and eliminating the branch entirely.
There was a problem hiding this comment.
I don't think get_parameter_or would eliminate the branch
instead you'd just end up with smth like
int thread_count;
node->get_parameter_or("thread_count", thread_count, 0);
if (thread_count > 0) {
// ...
} else {
// ...
}so, you still have the branch, it just removes the two parameter calls & replaces them with a single one.
though ig at that point maybe you could do a ternary for exec? so like,
int thread_count;
node->get_parameter_or("thread_count", thread_count, 0);
auto exec = thread_count > 0
? /* ... */
: /* ... */
(at least, I'm pretty sure this is what you'd get. I'm writing this on my phone and just looking at the docs for it)
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI.
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
|
Pulls: #3090
|
4 participants
Description
Avoids the unecessary creation of a
MultiThreadedExecutorin thecomponent_container_mtnode.The overhead of creating an unecessary
MultiThreadedExecutorhere is quite minimal, but there is some amount of overhead due toExecutor's constructor.I just noticed this and thought I'd contribute this, even though quite minor.
Also, I added
consttonode, as afaik there's no reason not to here.Is this user-facing behavior change?
No.
Did you use Generative AI?
No. I don't use slop bots.