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

Deprecate StaticSingleThreaded Executor. #4812

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

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented Oct 9, 2024

we should also deprecate StaticSingleThreadedExecutor from the documentation.

aligns with ros2/rclcpp#2598

Important

Do not backport this, because StaticSingleThreadedExecutor is deprecated only in rolling.

@fujitatomoya
Copy link
Collaborator Author

@clalancette @alsora could you review this?

Copy link

github-actions bot commented Oct 9, 2024

HTML artifacts: https://github.com/ros2/ros2_documentation/actions/runs/11670308253/artifacts/2142625163.

To view the resulting site:

  1. Click on the above link to download the artifacts archive
  2. Extract it
  3. Open html-artifacts-4812/index.html in your favorite browser


The *Static Single-Threaded Executor* has been deprecated, and *Single-Threaded Executor* is recommended instead.
The *Static Single-Threaded Executor* was the only *Executor* optimized the runtime costs for rebuilding the entities of a node in terms of subscriptions, timers, service servers, action servers, etc.
But this runtime improvements are now available in all the other *Executor*, so that all *Executor* can now take advantage of this improvement.
Copy link
Contributor

@alsora alsora Oct 9, 2024

Choose a reason for hiding this comment

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

I think the two sentences here can be written in a more clear way.

The Static Single-Threaded Executor was the only Executor optimized the runtime costs for rebuilding the entities of a node in terms of subscriptions, timers, service servers, action servers, etc.
But this runtime improvements are now available in all the other Executor, so that all Executor can now take advantage of this improvement.

could become

The Static Single-Threaded Executor was developed to reduce the the runtime costs for scanning the entities of a node in terms of subscriptions, timers, service servers, action servers, etc.
These runtime improvements are now available also in all the other Executor.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also mention that there are known bugs affecting only the static executor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alsora do you mean ros2/rclcpp#2589, right? i am not sure about that, because that looks like regression and we are going to fix that? if it seems to take much longer time to address it, probably we can consider that with another PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alsora friendly ping.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that ticket refers to a bug affecting all waitset-executors, but there are also some bugs that are specific to the static executor.

You can see them looking at the unit-tests that are run on StandardExecutors (i.e. they are not run on the static executor)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alsora i added b2adf9d, can you check and approve?

Signed-off-by: Tomoya.Fujita <[email protected]>
Copy link
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.

This looks good to me, but I'd like to get @alsora 's approval before we merge.

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

Successfully merging this pull request may close these issues.

3 participants