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

Add option to specify a message type #1153

Merged
merged 6 commits into from
Jan 6, 2023

Conversation

carlossvg
Copy link
Contributor

This PR adds an option to the Performance benchmarking package to use a predefined message type instead of generic type. The motivation for this option is to allow to replicate scenarios with exactly the same message types as the original case.

@carlossvg carlossvg requested a review from a team as a code owner November 8, 2022 10:57
@carlossvg carlossvg requested review from gbiggs and MichaelOrlov and removed request for a team November 8, 2022 10:57
@carlossvg carlossvg force-pushed the add-message-type-option branch 3 times, most recently from be85f63 to adab4e6 Compare November 15, 2022 11:30
Signed-off-by: Carlos <carlos.sanvicente@apex.ai>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
This reverts commit d07f4a8.

Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
@carlossvg carlossvg changed the title Draft: Add option to specify a message type Add option to specify a message type Nov 17, 2022
@carlossvg
Copy link
Contributor Author

@emersonknapp @adamdbrw Could you please review this PR? As explained in the description the motivation of this PR is to be able to run benchmarks using different message types instead of a generic type. The main reason would be to be able to reproduce as close as possible different scenarios.

For this reason a new option, msg_type, is introduced to specify the message type we want to use. For the moment only sensor_msgs` are included but the idea would be to provide some instructions so people can add their own message types.

A logical question is what happens now when msg_type is used with msg_size_bytes. Here there several cases:

  • If msg_type is not used the message type is defaulted to the byte array message type. The behavior should be the same as before.
  • If msg_type is used and there is no msg_size_bytes the message is not resized. This makes sense for fixed size messages (i.e. std_msgs::msg::Int32).
  • If msg_type is and a non-zero msg_size_bytes the developer should add a function to construct and resize tat message type. In case the message type has multiple bounded/unbounded fields is not obvious how msg_size_bytes should be used. I suggest to assume we want to resize all the fields in the same proportion so the total message size is msg_size_bytes.

Additional notes:

  • Currently, only benchmark_publishers support the msg_type option. A suitable error message should be added to inform the user.
  • We added automotive.yaml to show a usage example.
  • We created rosbag2_performance_benchmarking_msgs to remove the std_msgs dependency and to provide a place to add more message types if needed in the future (i.e. nested types).

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@carlossvg Thanks for the PR.
In general it looks good to me among a few nitpicks.
I would really appreciate if you will address them.


void generate_data(sensor_msgs::msg::Image & msg, size_t size)
{
// TODO(carlossvg): calculate message base size to set total size
Copy link
Contributor

Choose a reason for hiding this comment

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

@carlossvg Can you please clarify what this TODO about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichaelOrlov Currently the total message size will be all the message fixed size fields (https://github.com/ros2/common_interfaces/blob/rolling/sensor_msgs/msg/Image.msg#L4-L25) + the size of the data field (which is the size parameter passed as a function argument). Ideally the total message size would be equal to size. In order to do that we would have to calculate the size of all other fields except from data and then subtract this value to the size parameter to resize data the necessary amount.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add meaningful comment to the todo with the same level of details?

@MichaelOrlov
Copy link
Contributor

@carlossvg Kindly ping to reiterate on this PR.

Signed-off-by: Carlos San Vicente <carlos.sanvicente@apex.ai>
@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/7374b636991bac95242b44bc7924e462/raw/192fd2e9264b5bdc3ff901c799249cc8e5de6376/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_performance_benchmarking rosbag2_performance_benchmarking_msgs
TEST args: --packages-above rosbag2_performance_benchmarking rosbag2_performance_benchmarking_msgs
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11344

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

@MichaelOrlov
Copy link
Contributor

Failure in Test rosbag2 / build_and_test (pull_request) is unrelated to the changes from current PR and relates to the moved mcap packages.

  Package 'mcap_vendor' specified with --packages-up-to was not found
  Package 'rosbag2_storage_mcap' specified with --packages-up-to was not found
  Package 'rosbag2_storage_mcap_testdata' specified with --packages-up-to was not found

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.

2 participants