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

Do not force asynchronous publishing #343

Closed

Conversation

MiguelCompany
Copy link
Collaborator

Since eProsima/Fast-DDS#898, asynchronous publishing is not necessary by default. This PR lets the default value for the publishing mode (which is synchronous), which usually has better latency results.

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Left a question, otherwise LGTM!

README.md Show resolved Hide resolved
@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Jan 13, 2020
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Can you apply the same three changes to rmw_fastrpts_dynamic_cpp (rmw_publisher, rmw_client, rmw_service)?

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Copy link
Collaborator Author

Can you apply the same three changes to rmw_fastrpts_dynamic_cpp (rmw_publisher, rmw_client, rmw_service)?

ups! done!

@ivanpauno
Copy link
Member

ivanpauno commented Jan 13, 2020

CI including test_rclcpp, test_communication:

  • Linux Build Status (unrelated failure)
  • AArch64 Build Status (unrelated failure)
  • OSX Build Status (unrelated failure)
  • Windows Build Status

@JaimeMartin
Copy link
Contributor

@ivanpauno is ok with you if we merge this? We need this change for the ongoing performance tests.

@ivanpauno
Copy link
Member

@ivanpauno is ok with you if we merge this? We need this change for the ongoing performance tests.

I haven't merged yet, because I'm not sure of all the implications this PR has.
About performance tests, I think the order is the opposite. First, have a performance test framework that can measure performance of what we currently have. Then do performance improvements, and use the performance tests as a proof. Without performance tests, I have to blindy trust that performance does improve with this PR.


I still think the changes are reasonable. Adding @wjwwood for a second approval before merging.

@JaimeMartin
Copy link
Contributor

The performance improves, yes. We have your performance tests replicated on our performance test machines. You can safely merge this.

@wjwwood
Copy link
Member

wjwwood commented Jan 15, 2020

Please do not merge this until I have time to review it.

I think this may be changing behavior in a significant way and is also causing the behavior of publish to be different from other rmw implementations.

Also I think that allowing the publishing mode to be changed via external xml is a very bad idea as I detailed in this discussion:

ros2/ros2#255

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

The code changes look fine, but I have to disagree with the change in general.

We picked asynchronous publishing in the past, see:

Given how poorly it is documented and the indecision we've had (again see ros2/ros2#255), it doesn't surprise me that we have such an even split in implementations. But I will open issues on the latter two just so they're aware.

We did this to more closely emulate ROS 1's behavior (where published data was always just pushed onto a queue(s) which was processed separately, see https://github.com/ros/ros_comm/blob/d9059f49d14b7baf4a58c7759deab1f019681f5b/clients/roscpp/src/libros/publication.cpp#L407-L431). Also, many implementations require asynchronous publishing to handle large data (fragmentation).

For the above reasons I think it makes sense to stay with asynchronous publishing right now.

I could see a few paths in the future:

In all of those cases we need to document the expected blocking behavior of the rmw_publish() function (and derived functions).

Personally I like the idea of adding an option to the ROS api in this case, but that's not likely to get back ported or be implemented very quickly. See also ros2/ros2#255

I would be happy to hear from others in the community and on the @ros2/team, to see if they agree or disagree with me.

@tfoote
Copy link

tfoote commented Jan 23, 2020

I agree that we should stick with asynchronous across all implementations for consistency between implementations as well as because it's what's expected of the ROS API. We were actually inconsistent between python and c++ originally in ROS 1 and converted python to asynchronous several years ago too.

From the comment on the Opensplice ticket it appears that it's also asynchronous and doesn't have support for synchronous publishing ros2/rmw_opensplice#301 (comment)

@MiguelCompany
Copy link
Collaborator Author

Closing this, as we already provided a way of avoiding async publishing on #470

@MiguelCompany MiguelCompany deleted the feature/dont_force_async_publishing branch April 6, 2021 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants