Issue #15: rxros::spin() function uses unnecessary many threads #30
Conversation
|
@henrik7264: you should rebase your PR once #33 gets merged. |
rxros/include/rxros/rxros.h
Outdated
| static void spin() { | ||
| ros::MultiThreadedSpinner spinner(0); // Use a spinner for each available CPU core | ||
| static void spin(uint32_t thread_count = 1) { // Use per default 1 thread for spinning | ||
| ros::MultiThreadedSpinner spinner(thread_count); |
There was a problem hiding this comment.
Something to consider perhaps (again): is there benefit to having a spin(..) in rxros? If we allow use of regular ros::spin()/MultiThreadedSpinner/AsyncSpinner instead, we would not need to maintain / debug this API and could reuse existing documentation and infrastructure.
There was a problem hiding this comment.
I tend to side with @gavanderhoorn here. I checked the implementation of spin in rxros.h and it appears it does not do anything special. We are only adding ourselves work to maintain it. Unless there are plans that it should do more and these plans make sense to handle on rxros side, not in some other library. Do you have any plans @henrik7264 ?
There was a problem hiding this comment.
The initial idea with the rxros::spin() function was to free the programmer from having to think about how many threads should be allocated to handle the topic queues. The result was however that too many threads were allocated especially for simple nodes which only needs a single thread for handling the topics.
I agree you that the current solution may lead to some confusion, so I will remove the rxros::spin function. In addition I will remove the rxros::init and rxros::ok. They are all nothing but simple wrapper functions.
The second idea with the wrapper functions was to provide a simplified/unified interface to ROS. The overall goal was that the RxROS programmer should use nothing but functions like rxros::something::something_else. The RxROS programmer should always think: How can I solve my problem using RxROS. If we remove the simple wrapper functions we end up with a mix of ROS and RxROS functionality in every program, and I fear this will lead to another mindset of the programmer. Well, I don't know if this gives any meaning to you.
There was a problem hiding this comment.
I will start on the update as soon as you agree on removing
rxros::init
rxros::ok
rxros::spin.
There was a problem hiding this comment.
If those wrappers do not add any functionality right now, I would agree to remove them.
The second idea with the wrapper functions was to provide a simplified/unified interface to ROS. The overall goal was that the RxROS programmer should use nothing but functions like rxros::something::something_else.
that is a valid rationale for having these wrappers. But as you've noticed, creating them also incurs maintenance overhead.
If we really want/like this, you could take a look at creating some function aliases (using using). They would alias the regular versions, but everything (so, documentation, behaviour, return values, etc, etc) would be completely identical.
There was a problem hiding this comment.
I am sensitive to Henrik's argument. Mixing the two programming styles is quite a problem. OTOH, I am willing to wait with introducing these functions until we understand what they should do besides wrapping.
BTW. Does not rxros::init do a bit more in fact? (creating a node object as well?) I did not check - not time right now.
There was a problem hiding this comment.
OK. I will start to remove them - Observe that this change will impact both the examples and rxros header and readme file.
I could clean up the library even further by removing the rxros::parameter and rxros::logging functions. This would leave a very clean library consisting of nothing but the rxros::observable and rxros::operator fuctions. This is fully in the spirit of reactive programming and RxCpp.
@wasowski: rxros::init does setup/create the node, but it does it simply by calling ros::init.
gavanderhoorn
force-pushed
the
rxros_spin
branch
from
66b259e
to
8202ed8
Compare
|
Guys: with ROSCon next week it would be good to come to a conclusion here (so @wasowski would have time to update his slides as well). Either we merge this as-is and decide about dropping @henrik7264: what would have your preference? |
Removed rxros::init, rxros::spin and rxros::ok.
|
I have commited an update where the rxros::init, rxros::ok and rxros::spin have been removed. I would actually have like also to remove the logging and parameter class as proposed above, but we can talk about this after the conference. If we merge this commit to master and release it, @wasowski must update his notes. I think it is relative safe to release it. I shall make some tests tomorrow. Please, @gavanderhoorn and @wasowski take a look at the Readme. It was put together a bit fast. |
|
Please also remember that this commit has impact on the RxROS examples. |
|
I merged since, it did not seem to require a rebase. |
Fix of Issue #15: rxros::spin() function uses unnecessary many threads.
Default is now set to one thread and the programmer can if needed specify more threads by giving an argument to the spin function.