deprecate rclcpp::spin_some and rclcpp::spin_all #2848
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alsora
force-pushed
the
asoragna/deprecate-global-spin-some-all
branch
from
4bf1cb8 to
e8e8faf
Compare
…match the other executors Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
alsora
force-pushed
the
asoragna/deprecate-global-spin-some-all
branch
from
e8e8faf to
f1e7310
Compare
mjcarroll
approved these changes
May 17, 2025
| #else // !defined(_WIN32) | ||
| # pragma warning(push) | ||
| # pragma warning(disable: 4996) | ||
| #endif |
There was a problem hiding this comment.
I added a macro for this in cpputils
There was a problem hiding this comment.
which is ros2/rcpputils#210
| #else // !defined(_WIN32) | ||
| # pragma warning(push) | ||
| # pragma warning(disable: 4996) | ||
| #endif |
There was a problem hiding this comment.
which is ros2/rcpputils#210
|
|
||
| auto request = std::make_shared<test_msgs::srv::Empty::Request>(); | ||
| auto future = empty_client->async_send_request(request); | ||
| state.ResumeTiming(); | ||
| benchmark::DoNotOptimize(service); | ||
| benchmark::ClobberMemory(); | ||
|
|
||
| rclcpp::spin_until_future_complete(node->get_node_base_interface(), future); |
There was a problem hiding this comment.
do we need deprecate this too?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Deprecate
rclcpp::spin_someandrclcpp::spin_all.These functions can lead to very poor performance for developers who are not aware that calling them will:
If called in a loop, you are wasting a lot of CPU cycles repeating those operations every time.
This instead is a lot more efficient because only the spin operation is repeated as expected.
Adding and removing nodes from executors are expensive operations that shouldn't be done at "steady state" of the system.
IMPORTANT
In this PR I'm also re-ordering the events-executor constructor arguments so that this becomes possible:
Now all executors take the executor options as first argument.
Is this user-facing behavior change?
Yes, deprecating some functions and modifying a constructor (although it's in the experimental namespace)
Did you use Generative AI?
No
closes #2675