Reduce lifecycle manager nodes

[gezp]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	(tickets: #816)
Primary OS tested on	(Ubuntu20.04, ROS 2 Galactic)
Robotic platform tested on	(gazebo simulation of turtlebot3)

---

Description of contribution in a few bullet points

As described in #816, bond_client_node_ in class LifecycleManager need be removed, you can find more details(other nodes which need be removed) in #816 (comment)

I use CallbackGroup + MultiThreadedExecutor instead of bond_client_node_.

• because Bond doesn't support new callback group, so let Bond use default_callback_group, and create new callback group for services and timers in LifecycleManager.
• then use MultiThreadedExecutor with 2 threads to spin 2 CallbackGroup in main()

Test

#colcon test
colcon test --packages-select nav2_lifecycle_manager

#result:0 errors, 0 failures, 0 skipped

For Maintainers:

• Check that any new parameters added are updated in navigation.ros.org
• Check that any significant change is added to the migration guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[SteveMacenski]

Love it, no issues

[wjwwood]

The changes lgtm, but I want to point out how this changes the threading architecture for your application, which might be intentional or not.

The small difference here is that before this change you had all the things associated with the bond_client_node_ in its own thread (at least that's what I assume nav2_util::NodeThread is doing) and then the main thread was used for the things associated with the nav2_lifecycle_manager::LifecycleManager node. But now you have two threads calling callbacks, but those threads may call things from either the bond_client_node_ (now the callback_group_ in the lifecycle manager) or the nav2_lifecycle_manager::LifecycleManager, which effectively means you no longer have a dedicated thread for each. This might be fine, but is a change in how execution was set up before.

You could, alternatively, create the callback group with the second constructor argument as false (see: https://github.com/ros2/rclcpp/blob/0d6d9e67782bcd1de06af875459e4abb2b91ab86/rclcpp/include/rclcpp/callback_group.hpp#L89-L96), which would prevent it (and anything added to that callback group) from being handled by rclcpp::spin(node) (or equivalent) and you could spin your own thread with an executor just for that callback group. This would maintain a similar thread structure to what you had before but still remove the redundant node.

[wjwwood]

In case it wasn't clear what I was talking about, I made gezp#1. Feel free to take that or not, what you have here is not wrong, I just wanted to show you the other option.

[SteveMacenski]

Would it not be true that if we have a multi-threaded spinner with 2 threads and 2 MutuallyExclusive callback groups that in essence one thread would belong to each callback group? If they are mutually exclusive, the bond callback group could not run in both threads without violating the callback group type. Same with the services. Surely they could swap threads at pseudo-random but since any single one of them cannot be running in both, then there is always 1 thread open to receiving messages from the other callback group at any given time (assuming not already processing a request from both callback groups)?

Separately, what is your thought about, all else equal, the differences in performance between 2 single threaded executors and 1 multi-threaded executor of 2 threads? Essentially that's what I understand your suggestion to boil down towards. If the above suggestion is true, then really the choice between the options should come down solely to whichever is better practice and/or better performance. The multithreaded executor feels more correct, but that's just my intuition, you're the actual master of ROS2 stack to dictate that design pattern.

[wjwwood]

Would it not be true that if we have a multi-threaded spinner with 2 threads and 2 MutuallyExclusive callback groups that in essence one thread would belong to each callback group?

No. MutuallyExclusive applies to callbacks within that callback group. Two ME callback groups operate independently of one another.

Also, mutually exclusive does not have anything to do with priority or thread affinity. So, you could have two callbacks from one callback group using the two threads and then callbacks in the other callback group would have to wait on them to free up a thread before being executed, and it does not guarantee that each time your callback gets called it will be on the same thread, it might swap between threads depending on which one is free at the time.

If they are mutually exclusive, the bond callback group could not run in both threads without violating the callback group type.

That is true, but there might be more than one callback group in the lifecycle manager node, and they could use both threads, delaying or starving (at least temporarily) the bond callback group.

Surely they could swap threads at pseudo-random but since any single one of them cannot be running in both, then there is always 1 thread open to receiving messages from the other callback group at any given time (assuming not already processing a request from both callback groups)?

Again, that assumes that what you created (in the lifecycle manager node or in the bond callback group) are the only entities that need to be called back or have callback groups of their own. For example, if use_sim_time is true then there will be a callback group for that and a callback to be called when messages are received on the /clock topic. That would potentially be using one of the two threads sometimes. That's just one example, there might be more in the future. (in that specific case, the use sim time case, there's an option for it to have it's own thread to avoid issues)

Separately, what is your thought about, all else equal, the differences in performance between 2 single threaded executors and 1 multi-threaded executor of 2 threads?

tl;dr

I would say the multi-threaded can be more efficient if one source of callbacks is very busy but the other is kind of idle, but this could be accomplished with a multi-threaded executor and a single threaded executor used in concert. The more important consideration is priority, i.e. is it important that one of them never be blocked or maybe delayed by callbacks from another group.

I think the situation you're trying to address, where you can wait on a service response from within a callback (if I understand correctly), is better served by using a dedicated thread/single-threaded executor for the service callbacks.

Using a dedicated executor for the service callbacks also makes your node more generic (in my opinion). Consider if someone else wrote a new main.cpp and instantiated the lifecycle manager node but put it in a single threaded executor. It might break, which sucks. We've discussed having the ability for nodes to express a requirement like "needs a multi-threaded executor" to try and avoid misuse like that, but for now, using a dedicated thread and executor inside the node makes it so that the node itself can work with any executor and no longer relies on how it is used in the main function. This is better in my opinion.

---

Well, the multi-threaded executor with two worker threads is actually two threads (though this is an implementation detail and might change in the future), because the thread in which you call spin() will serve as one "worker" and the other "workers" are spawned in the spin function first (number of threads - 1):

https://github.com/ros2/rclcpp/blob/0d6d9e67782bcd1de06af875459e4abb2b91ab86/rclcpp/src/rclcpp/executors/multi_threaded_executor.cpp#L57

There has been a proposal to change this so that the spin() thread that is blocking just waits for work and "dispatches" work to the worker threads, which has some benefits but would mean that there are number of workers + 1 threads. Again this isn't the case today, but might change.

So it's not more threads atm, so not more overhead there, but there is some overhead in keeping the threads in sync. Only one thread can wait for work at a time, and there are mutex to control this behavior, and it also uses the threads round-robin which means a context switch for each new callback.

In comparison, two single threaded executors are almost completely decoupled and do not need to be synchronized at the rclcpp layer at all. The middleware may need to do something at the bottom of the stack, but that is there in the multi-threaded one too. So there is some value in two single threaded executors, but the reason we have multi-threaded executors and the idea of "component nodes", is that if you create a thread and executor for every callback group (the logical extreme of this idea), then you end up with many many threads and the overhead of that becomes an issue. Which is one of the reasons thread-pools are so popular in the programming world (also to avoid creating/destroying threads, but that's a different issue).

So I'd say the multi-threaded is more overhead, but more throughput, and the two single-threaded are lighter weight, but could be less throughput in certain situations. But overall I'd say performance is not the primary considerations for this situation, instead making sure you don't have deadlocks or delays in important callbacks is the goal.

[wjwwood]

Consider if someone else wrote a new main.cpp and instantiated the lifecycle manager node but put it in a single threaded executor. It might break, which sucks.

If nothing else, having your node not rely on being used with a multi-threaded executor is a good reason to use a dedicated thread for the service callbacks.

[SteveMacenski]

No. MutuallyExclusive applies to callbacks within that callback group. Two ME callback groups operate independently of one another.

Ah, there lies my misunderstanding. From that point, I agree with the rest of what you mention.

Curious, is there a setting where each callback group acts like a separate "group" that are mutually exclusive to other groups? Basically being "externally" mutually exclusive vs "internally" mutually exclusive? It would be useful for the situation above, I have N needs for interfaces to run in parallel and I can craft M groups that contain the independent elements that need to run in parallel but will not directly interact with each other. This is most commonly found in ROS 2 nodes containing action or service servers where within the callback, we need to execute other callbacks or continue processing subscription sensor data to refine internal models.

This is just my opinion, but I find that choice of language confusing. If I have a callback group, I think of them as a bundle that are... well... grouped together. Then when we talk about mutual exclusivity, I then assume that we're applying mutually exclusivity to the group w.r.t. other groups, since groups are usually treated as an entity in common language use. Looking over the limited documentation on the subject, they do point to what you're mentioning so it is clearly my mistake, but I think potentially different words here could make it fit better into the vernacular definitions.

But anyway, that's all here nor there.

---

I would say the multi-threaded can be more efficient if one source of callbacks is very busy but the other is kind of idle

That would be consistent with this lifecycle manger application. Bond will be going constantly, but the actual lifecycle transitions will only happen on rare occasions (if a robot is doing a job, it should only need to come up or down 1 time if no issues, and only a small handful of times if there are issues).

To count the places I see where things are processing

• Lifecycle is active service
• Lifecycle manage state service -- the 2 services are never called at the same time in any existing Nav2 code, but a user could conceivably do so.
• Lifecycle manager clients to transition
• Bond alive sub/pubs - constant
• /clock - constant

But overall I'd say performance is not the primary considerations for this situation

Agreed, I'm usually more focused on readibility / understandability of code, through reasonable performance is also required, but this server isn't high traffic at all. It's one of the reasons I'm thinking hard about this server if we could change the callback groups to be structured such that a multithreaded spinner is the better option. I think 1 multithreaded spinner is easier for a newcomer (or myself in 2 years) to think about than trying to track exactly where multiple executors are being triggered / spun. This example server is not a problem since we're just opening a new thread and spinning it freely, but I'm thinking more about the precident of this for future servers where it won't be that simple to track. I'm thinking if we have to use 3 or more executors, where 2 or more of them are not free spinning and are spin_some in particular places. It's definitely more explicit and if done right more efficient, but that comes with more mental overhead to keep track of if trying to make significant changes.

Maybe that's just the mental overhead we need to add in order to best use the tools / make reliable code with ROS2, but if it can be avoided, I know Steve + 2 years from now will appreciate it 😆

[wjwwood]

Curious, is there a setting where each callback group acts like a separate "group" that are mutually exclusive to other groups?

When developing them originally, we thought about nesting callback groups, which would accomplish this, but we decided there wasn't a strong use case for it. If you have a ME group of ME callback groups, then you could simplify that as a single ME callback group. If your use case was that these two ME callback groups don't know about each other (developer separately but run together), then we couldn't come up with a reason for them to be ME, as the whole point of ME is to avoid resource contention, but if they don't know about each other then they cannot share a resource. If the resource sharing is global or something like that, then you could resource share the single ME callback group too. So we decided to keep the groups simple and not allow nesting. That being said, you can create your own callback groups as well. I'm not sure what issues might arise (the interfaces might need adjustment) but it was intended that users could make their own callback groups if they found new ways to organize things that had value.

but I think potentially different words here could make it fit better into the vernacular definitions.

I understand where you confusion comes from, but I don't know of another name that would be better suited.

Agreed, I'm usually more focused on readibility / understandability of code, through reasonable performance is also required, but this server isn't high traffic at all.

I was thinking more like "how to ensure callbacks are not deadlocking and/or starving other important tasks" is more important than performance here. Readability is also important, but I was trading-off results in the resulting class, i.g performance vs. safety (specifically that using the node with a single-threaded executor rather than a multi-threaded executor would be dangerous and there's no way to require that from the Node's perspective), rather than code readability.

[SteveMacenski]

LGTM

[SteveMacenski]

we thought about nesting callback groups

While I think that would be a solution such that you wouldn't need to create a new type of callback group, but I think what I am suggesting is a new type of callback group which is basically an inversion of the existing MutuallyExclusive group. In this group, callbacks would be grouped together to be run mutually exclusively to other callback groups but OK to run at the same time as other interfaces within the group. This is the "externally" mutually exclusive option. I think this is easier to reason about and largely what we need when building many servers (action, service) that need to be processing data.

That could be accomplished with nested groups but I agree that might be a bridge too far (for now, at least).

but I don't know of another name that would be better suited

I think if we ever plan to add more callback groups, I think the name could stick. If this is as many as there is ever planned to be, then I think callback_constraints is a decent option (not amazing, but a starting point for discussion). It gets across that you're declaring constraints to be applied to different interfaces. Then the context of MutuallyExclusive would be the right English definition of the word and basically the Reentrant one is the lack of all constraints. A new option like I detail above could then be a Group (probably needs a prefix word)