Skip to content

deprecate the static single threaded executor#2598

Merged
alsora merged 7 commits intorollingfrom
asoragna/deprecate-static-executor
Aug 20, 2024
Merged

deprecate the static single threaded executor#2598
alsora merged 7 commits intorollingfrom
asoragna/deprecate-static-executor

Conversation

@alsora
Copy link
Copy Markdown
Collaborator

@alsora alsora commented Aug 5, 2024

This PR marks the StaticSingleThreadedExecutor as deprecated.
With the current implementation, all other single threaded executors have better performance than this one and they are more stable.

See https://discourse.ros.org/t/the-ros-2-c-executors/38296 for a recent performance comparison that I run.

@alsora
deprecate the static single threded executor
2becd38 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
clalancette
clalancette reviewed Aug 5, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I'm totally on board with this deprecation, however:

  1. We are almost certainly going to have to make changes to the tests to either remove tests from StaticSingleThreadedExecutor, or suppress this warning (CI should show this).
  2. I'd like to get @wjwwood and/or @mjcarroll to approve.

mjcarroll
mjcarroll approved these changes Aug 5, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

I approve (and this was always the goal).

@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Aug 5, 2024

or suppress this warning

We should probably suppress the warning for this iteration before removing entirely next time (to stick with our tick-tock policy)

fujitatomoya
fujitatomoya approved these changes Aug 5, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

@alsora
suppress deprecation warning for static executor and remove benchmark…
312e40c 
… tests

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Aug 5, 2024 •
edited
Loading

I removed the performance benchmark tests and suppressed the deprecation warning for the unit-tests.
IMO the performance tests are not very useful at this point, but let me know if you prefer to keep those around with the deprecation suppressed.

@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Aug 7, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

clalancette
clalancette reviewed Aug 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

It looks like there are some uncrustify fixes that are needed, but otherwise this looks good to me.

@alsora
fix uncrustify linter errors
3edb511 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Aug 8, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Aug 8, 2024 •
edited
Loading

Pulls: #2598
Gist: https://gist.githubusercontent.com/clalancette/6e0606e387dfd155aea36c216fb104f7/raw/72dababdc6724085155820dafffe2865fc3e1c5a/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14387

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Aug 10, 2024

Retrying windows

  • Windows Build Status

Does anyone know how to see the warnings produced above?

@alsora
fix windows deprecation warnings
0d8e8dc 
i created an alias to give the deprecated executor a new name; this works when the class is directly used but it doesn't work in combination with templates (like std::make_shared<DeprecatedAlias>) because the compiler needs to resolved the type behind the alias triggering the warning

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Aug 10, 2024

Ok, I can see the windows warnings here https://ci.ros2.org/job/ci_windows/22373/msbuild/#issuesContent.
It looks like it's complaining that I'm using the deprecated class.
I suspect that it's because some tests are using the executor type within a template (e.g. std::make_shared<ExecutorType>() ) and this requires the windows compiled to "look inside" the alias, triggering the warning.
I tried a quick fix for that, if it still complains on windows i'll get rid of the alias and use more pragmas to suppress the warnings.

New CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ros-discourse
Copy link
Copy Markdown

ros-discourse commented Aug 15, 2024

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

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-16th-august-2024/39130/1

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Aug 19, 2024

So this generally looks good to me now, but the Rpr job has warnings related to deprecations of the StaticSingleThreadedExecutor. Interestingly, the CI job that @alsora ran does not show these warnings. Any idea what is going on there?

Alberto Soragna and others added 3 commits August 19, 2024 13:48
Merge remote-tracking branch 'origin/rolling' into asoragna/deprecate…
56fb683 
…-static-executor
@alsora
update test_reinitialized_timers.cpp to use the executor test utilities
e2b9e87 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
do not use executor pointer
77bd7e7 
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Aug 19, 2024 •
edited
Loading

@clalancette The warnings in the Rpr job were caused by a new test that was recently introduced and that was not using the executor_types.hpp utilities.
My rclcpp branch (used in the CI job) didn't include that new test, so it succeeded.

I merged with rolling to update my branch and fixed those new tests.
Now the Rpr job is happy.

Here a new CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@alsora
Copy link
Copy Markdown
Collaborator Author

alsora commented Aug 20, 2024

I'm going to merge this since everything is green now!

@alsora alsora merged commit dfaaf47 into rolling Aug 20, 2024
@alsora alsora deleted the asoragna/deprecate-static-executor branch August 20, 2024 03:13
@ros-discourse
Copy link
Copy Markdown

ros-discourse commented Aug 20, 2024

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

https://discourse.ros.org/t/the-ros-2-c-executors/38296/14

@ahcorde ahcorde mentioned this pull request Aug 20, 2024
Merged
@fujitatomoya fujitatomoya mentioned this pull request Oct 9, 2024
Merged
@sea-bass sea-bass mentioned this pull request Nov 29, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clalancette clalancette clalancette left review comments

@mjcarroll mjcarroll mjcarroll approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alsora @mjcarroll @clalancette @ros-discourse @fujitatomoya