-
Notifications
You must be signed in to change notification settings - Fork 418
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
Set the minimum number of threads of the Multithreaded executor to 2 #2030
Set the minimum number of threads of the Multithreaded executor to 2 #2030
Conversation
Signed-off-by: Alexis Paques <paa1ti@bosch.com>
The use of Instead, I would prefer having the application developer define the strict minimum amount of threads (with 2 by default) for reproducible results. Plus, for computation intensive software (reentrant data callbacks to start multiple long processing, maybe for CV), a user option to set the number of threads higher would be great (in that case as the number of cores). I understand this is a non-issue for power-users which will manage themselves the single executor for the app or use composition, but it will avoid a whole range of issues. (aka what I just had, someone having a deadlock for a software which works for everybody else). |
I would add a warning / error /something if a user tries to set the number of threads of this executor to 1. Besides this, the change looks good to me. |
Signed-off-by: Alexis Paques <paa1ti@bosch.com>
Signed-off-by: Alexis Paques <paa1ti@bosch.com>
The check for The case of 0 is not possible anymore as if |
number_of_threads_ = number_of_threads ? number_of_threads : std::thread::hardware_concurrency(); | ||
if (number_of_threads_ == 0) { | ||
number_of_threads_ = 1; | ||
number_of_threads_ = number_of_threads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: number_of_threads
is a size_t
, not a boolean, so prefer doing an explicit check with number_of_threads > 0
Signed-off-by: Alexis Paques <paa1ti@bosch.com>
4056eab
to
55de091
Compare
Signed-off-by: Alexis Paques <paa1ti@bosch.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me (though I'm going to update the title; we aren't setting the default to 2, we are setting the minimum to 2). I'll run CI on it next.
CI is happy here, so going ahead with this. Thanks for the contribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following up the review, lgtm
Signed-off-by: Alexis Paques paa1ti@bosch.com
Closes #2029
This ensures the number of threads of a Multi-threaded executor is at least 2 unless defined explicitly as 1 (why not use the SingleThreadedExecutor?)
Indeed, if we are calling a service from a callback and call the get function of the future directly, we effectively block until the service request resolves but it can't as the reply will not be processed, causing a deadlock. This is either solved by having a Multi-threaded executor and multiple callback groups for non time critical software (which has to wait for the result anyway).
The default number of threads is
std::thread::hardware_concurrency
which can be 1 for a VM or a small target.This means those targets (or VM) will not be able to receive the message of an otherwise correct node, while working on everybody else's machine.
The second motivation is to solve the misconception a Multithreaded executor always has multiple thread, by making it have always multiple treads by default.