Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Thread configuration prototype #2205

Open
wants to merge 5 commits into
base: rolling
Choose a base branch
from

Conversation

smorita-esol
Copy link

This is a prototype implementation of RCLCPP for discussion about the thread configuration feature to receive and apply a set of scheduling parameters for the threads controlled by the ROS 2 executor.

Our basic idea is as below.

  1. Implement a new class rclcpp::thread and modify rclcpp to use it. This class has the same function set as the std::thread but also additional features to control its thread attributions.
  2. Modify the rcl layer to receive a set of scheduling parameters. The parameters are described in YAML format and passed via command line parameters, environment variables, or files.
  3. the rclcpp reads the parameters from rcl and applies them to each thread in the thread pool.

There have been some discussions about this pull request, as below.
[ROS Discourse]
https://discourse.ros.org/t/adding-thread-attributes-configuration-in-ros-2-framework/30701

[ROS 2 Real-Time Working Group]
ros-realtime/ros-realtime.github.io#18

This is a prototype implementation of RCLCPP for discussion about the thread configuration feature to receive and apply a set of scheduling parameters for the threads controlled by the ROS 2 executor.

Our basic idea is as below.
 1. Implement a new class rclcpp::thread and modify rclcpp to use it.
   This class has the same function set as the std::thread but also additional features to control its thread attributions.
 2. Modify the rcl layer to receive a set of scheduling parameters.
   The parameters are described in YAML format and passed via command line parameters, environment variables, or files.
 3. the rclcpp reads the parameters from rcl and applies them to each thread in the thread pool.

There have been some discussions about this pull request, as below.
[ROS Discourse]
https://discourse.ros.org/t/adding-thread-attributes-configuration-in-ros-2-framework/30701
[ROS 2 Real-Time Working Group]
ros-realtime/ros-realtime.github.io#18

Signed-off-by: Shoji Morita <[email protected]>
@smorita-esol smorita-esol force-pushed the origin/sched_param_control_for_osrf branch from 9487724 to 66e2d64 Compare June 6, 2023 09:53
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/adding-thread-attributes-configuration-in-ros-2-framework/30701/2

@smorita-esol
Copy link
Author

https://build.ros2.org/job/Rpr__rclcpp__ubuntu_jammy_amd64/902/
This error was caused by the dependency on the update included in the other pull request for rcl below.
ros2/rcl#1075

@alsora
Copy link
Collaborator

alsora commented Jun 6, 2023

This is a great feature, but I wonder if it should go somewhere else, e.g. rcpputils.
in particular all the stuff that is OS-dependent.

@mjcarroll
Copy link
Member

This is a great feature, but I wonder if it should go somewhere else, e.g. rcpputils.

Agree on both counts. rcpputils seems like an ideal candidate here.

@smorita-esol
Copy link
Author

smorita-esol commented Jun 7, 2023

@alsora, @mjcarroll
Thanks for your comments.

I would like to confirm one thing below to reflect your point about this pull request.

[Item to be confirmed]
Is it acceptable in terms of the design aspect of rclcpprcpputils to add an additional package dependency on the rcl package?

[Background]
In the pull request, the thread control code depends on the data structure (e.g. rcl_thread_attr_t) newly added to the rcl package in the pull request below.

ros2/rcl#1075

Our team is concerned that if the package dependency of rclcpprcpputils cannot be changed due to its design policy, we will have to move the data structure to rcutils as you point out for this pull request.

@clalancette
Copy link
Contributor

Is it acceptable in terms of the design aspect of rclcpp to add an additional package dependency on the rcl package?

rclcpp already depends (quite heavily) on rcl:

<depend>rcl</depend>

@smorita-esol
Copy link
Author

Is it acceptable in terms of the design aspect of rclcpp to add an additional package dependency on the rcl package?

rclcpp already depends (quite heavily) on rcl:

<depend>rcl</depend>

Thanks for your pointing out. And sorry for my misspelling.

I didn't mean rclcpp, but rcpputils, and I modified the comment above.

@alsora
Copy link
Collaborator

alsora commented Jun 8, 2023

I think that part of the rcl PR (e.g. the new data structure) can go to the rcutils library (this is the C equivalent of rccputils

@clalancette
Copy link
Contributor

I didn't mean rclcpp, but rcpputils, and I modified the comment above.

That's fine, rclcpp already depends on rcpputils as well:

<depend>rcpputils</depend>

@wjwwood
Copy link
Member

wjwwood commented Jun 15, 2023

rcpputils, however, cannot depend on rcl.

@wjwwood
Copy link
Member

wjwwood commented Jun 15, 2023

As the other said, I think this does not belong in rcl or rclcpp as it is not really ROS related, and ideally it would exist completely in user space, i.e. above rclcpp. If it needs to influence how rclcpp creates threads, then we can change rclcpp so that's possible, but not by adding this directly into it.

Adding things to help implement this in rcutils or rcpputils is fine, but only if the things you're adding are truly reusable in other situations, otherwise it should go in some other package that's focused on this use case.

smorita-esol added a commit to esol-community/rcutils that referenced this pull request Jun 20, 2023
smorita-esol added a commit to esol-community/rcpputils that referenced this pull request Jun 20, 2023
smorita-esol added a commit to esol-community/rcl that referenced this pull request Jun 20, 2023
@smorita-esol
Copy link
Author

Thanks for your suggestion.

Adding things to help implement this in rcutils or rcpputils is fine, but only if the things you're adding are truly reusable in other situations, otherwise it should go in some other package that's focused on this use case.

As you mentioned above, I've moved the proposed features from rcl and rclcpp to rcutils and rcpputils, respectively.
And our current implementation includes some incomplete code for the Windows platform. So, I will soon remove the code so as to make the rclcpp::thread act the same as the original std::thread.

smorita-esol added a commit to esol-community/rcpputils that referenced this pull request Jul 12, 2023
Please, refer to the comment below.
ros2/rclcpp#2205 (comment)

Signed-off-by: Shoji Morita <[email protected]>
std::lock_guard wait_lock{wait_mutex_};
for (; thread_id < thread_attributes_->num_attributes - 1; ++thread_id) {
thread_attr.set_thread_attribute(
thread_attributes_->attributes[thread_id]);
Copy link

@JanStaschulat JanStaschulat Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Multi-Threaded Executor has multiple threads as a thread-pool to process the callbacks of new incoming messages (timers, services, etc.) in parallel. However, the Executor does not differentiate between different messages or timers. At run-time, a callback could be processed by any thread. There is no mapping of e.g. important callbacks to high priority threads possible. So, in my opinion, it would only makes sense to configure all threads with the same configuration (e.g. priority, core-affinity, ...). If at all.

What is your intention to assign different thread parameters (e.g. priority, core-affinity) to different threads?
Could you give an example how you intend to use the Multi-Threaded Executor with different thread priorities in your use-case?

Copy link

@JanStaschulat JanStaschulat Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your great effort to add thread configuration to ROS2. This is very important piece of work regarding real-time support. I think we need a proper way to control the thread configuration of the thread, which is processing a callback. I am not so sure, that assigning different thread-priorities to to threads of the Multi-Threaded Executor solves the problem. It is also necessary to define a mapping of callbacks (of subscriptions, timers, services, etc.) to a particular thread configuration (e.g. priority).

Alternative approaches:

  • create a thread outside ROS 2 and then let the Single-Threaded Executor run inside it.

  • the user specifies the priority per subscription. Then the Executor changes the thread priority just before the callback is called (this concept works for the Single-Threaded as well as Multi-Threaded Executor. This concept will be used in an experimental real-time executor at the "Real-time programming with ROS 2" Workshop at ROSCon. The source code will be made available.

  • extend Events-Executor to control real-time behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other approaches are to create a thread outside ROS 2 and then let e.g. the single threaded Executor run.

I agree with this.
We should not have executors modify the attributes of the thread that they are running into. If users want to run a single-threaded executor in a thread with specific attributes, they can easily do it themselves, rather than having the executor change things under their feet.

A different scenario is whenever the rclcpp library internally creates threads (e.g. the events-executor, the clock subscriptions, the graph updates etc) all these can take advantage of the new tools to configure threads (rather than just blindly inheriting the parent thread configurations).

Besides that, I'm not convinced by the use of

rcutils_thread_attrs_t * attrs = rcl_context_get_thread_attrs(options.context->get_rcl_context().get()

We shouldn't restrict to a single thread attribute per ROS context and the two concepts shouldn't be tied together.
I would recommend to have all classes that deal with threads to take a rcutils_thread_attrs_t as input in the constructor

Copy link
Collaborator

@alsora alsora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this is the correct way to use the new thread attribute utilities.
See https://github.com/ros2/rclcpp/pull/2205/files#r1293481529 for details

@smorita-esol
Copy link
Author

smorita-esol commented Aug 23, 2023

@JanStaschulat

What is your intention to assign different thread parameters (e.g. priority, core-affinity) to different threads?
Could you give an example how you intend to use the Multi-Threaded Executor with different thread priorities in your use-case?

There are some use cases assigning each thread to specific cores to limit the CPU usage of a particular node (e.g., a node should run on energy-efficient cores). But, as you mentioned, setting different priorities and scheduling policies for the threads is redundant and meaningless.
So, what if we modify the current modification to reject such a setting when it has multiple priorities or just punch out a warning message. (Q1)

It is also necessary to define a mapping of callbacks (of subscriptions, timers, services, etc.) to a particular thread configuration (e.g. priority).

I agree that there are some cases like you mentioned. But still, I'm not sure if it is enough or not because there seem to be cases we should assign priorities for the event chain, not an event. I'd like to discuss this topic in the other thread.

create a thread outside ROS 2 and then let the Single-Threaded Executor run inside it.

I agree that there might be cases that require such a style. On the other hand, there seem to be some users who want to delegate such a thread creation to the executor.
And I understand the opinion of @alsora below that the executor must not alter the currently running threads and should create the threads it will use internally.

We should not have executors modify the attributes of the thread that they are running into. If users want to run a single-threaded executor in a thread with specific attributes, they can easily do it themselves, rather than having the executor change things under their feet.

So, what if we modified the implementation to create all the thread(s) the executor uses only when there are thread attributes passed to rcl, or the user application constructs the executor with thread attributes as a parameter, as I mentioned below? (Q2)

@alsora

We shouldn't restrict to a single thread attribute per ROS context and the two concepts shouldn't be tied together.

I could not understand what you meant to say above. Did you mean that we could have multiple executors per process, so we have to have multiple thread configuration sets for such cases? (Q3)

I would recommend to have all classes that deal with threads to take a rcutils_thread_attrs_t as input in the constructor

I agree with you. So, what if we modify the single/multi-thread executor to accept the thread attributes (rcutils_thread_attrs_t) and use it to set each thread attribution? (Q4)
This is useful when we have multiple executors per process because we can create an application to get the global thread parameters for the process via the rcl_context_get_thread_attrs() and divide it to each executor.
On the other hand, it is usual that we have only one executor for the process. So, I want to have the single/thread executors get the thread attributes via rcl_context_get_thread_attrs() as the current implementation in the PR.
This option is useful because we can use the thread attribute configuration infrastructure with the existing executor's usage, and we can also apply the infrastructure for the multiple executor case.

Incidentally, I'm going to implement the parameter passing mechanism above by modifying the ExecutorOption to have thread attributes configuration as below and also add a parameter overwriting method to set arbitrary thread attributes like NodeOptions.

// rclcpp/rclcpp/include/rclcpp/executor_options.hpp
namespace rclcpp
{
/// Options to be passed to the executor constructor.
struct ExecutorOptions
{
 ...(snipped)...
 rclcpp::memory_strategy::MemoryStrategy::SharedPtr memory_strategy;
 rclcpp::Context::SharedPtr context;
 size_t max_conditions;
 std::vector<rcpputils::Thread::Attribute> thread_attrs; // <-Added 
};

@alsora
Copy link
Collaborator

alsora commented Aug 23, 2023

TL;DR;
IMO the ROS library should provide the tools (a thread attributes class) and the way for using them (constructors take thread attributes as argument), but it should try to not make assumptions on how people are going to use those, which inevitably penalize whoever wants to do things differently (e.g. let people create their threads, decide how to pass the thread attribute class to each executor and so on).

My thoughts on Q1/Q2.
The multi-threaded executor is IMO a quick/convenient utility, but it's far from ideal if someone is seriously interested about performance (there's several performance analysis that show how a multithreaded executor with N threads is often less efficient than N single-threaded executors).
If someone needs to do core-pinning or other "advanced" optimizations, IMO they would be better not using the multi-threaded executor and handle individual threads and callback groups on their own.

create a thread outside ROS 2 and then let the Single-Threaded Executor run inside it.

I agree that there might be cases that require such a style. On the other hand, there seem to be some users who want to delegate such a thread creation to the executor.

it's the first time I hear about a real need for this use-case (people really want to have ROS create threads under their feet, when they could just to it with a single line of code? If they know what a thread is and they also have interest in configuring it, then I'm sure they would be happy to have full control over it). Moreover, I would rather have an utility that creates a thread with an executor for you, rather than adding a lot of complexity to the executor class itself.
And YES, I really mean a lot of complexity: handling exceptions thrown by user callbacks (could spin exit with the thread still running?), avoiding undefined behavior during destruction of the executor (can I safely destroy or is the thread running?), thread-safety, etc all become a lot harder if the executor class is internally creating a thread. We are facing this exact issue with the events-executor, which creates an additional thread internally. So I STRONGLY recommend to not make this change in the single-threaded executor, where it's absolutely not needed.

Did you mean that we could have multiple executors per process, so we have to have multiple thread configuration sets for such cases? (Q3)

Yes, having multiple nodes and executor per process is actually a common usage and it's encouraged by the presence of ROS 2 components.

So, what if we modify the single/multi-thread executor to accept the thread attributes (rcutils_thread_attrs_t) and use it to set each thread attribution? (Q4)

Yes, sounds good, but with some caveats.
I don't see the point of having a "global" thread configuration, accessible via rcl_context_get_thread_attrs() or similar APIs.
This is usable only if you have a single thread in your process (which I would argue could be common for toy examples, but very unreasonable in a real C++ robotics application).
If you get a std::vector<rcpputils::Thread::Attribute> thread_attrs how are you going to know which config should be used for which thread?
IMO the action of (quote) "dividing attributes to each executors" is something that really depends on the specifics of the application and it's not going to be useful to try to enforce a common pattern here (e.g. in one application I may want to use a map where each key is a thread name and each value is a thread configuration; in another I may have a special convention for which I have 10 threads and only 2 configurations: the first used by 5 and the second used by the other 5; all this to show that there may be countless of approaches to handle this)

So, I would say that we could expand the executor options with the new argument, but this should be a single value (not a vector/list/map). If the executor is going to create threads internally, then it's going to use this attribute for all its internal threads.
This means that multi-threaded executor and events-executor are going to use it, but the argument would be unused for single-threaded executor.

@alsora
Copy link
Collaborator

alsora commented Mar 26, 2024

Hi @smorita-esol, what's the current status of this PR?

@smorita-esol
Copy link
Author

This PR is based on the other PRs and REP, which are still discussed in the real-time Working group. Particularly, the REP-2017 is important to be accepted before the related PRs below get reviewed and merged.

ros2/rcl#1075
ros2/rcutils#424

While waiting for the acceptance, we might have to discuss the modification of the language binding layer (rclcpp) and/or create another REP related to this and rcpputils PR below.

ros2/rcpputils#179

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants