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 computation of size contribution to info verb #1726

Merged
merged 7 commits into from
Jul 23, 2024

Conversation

nicolaloi
Copy link
Contributor

@nicolaloi nicolaloi commented Jun 25, 2024

Addresses #1601 feature request (Show size contribution of each topic with ros2 bag info). This is a test draft.

Output of ros2 bag info test_bag -v --size-contribution:

Files:             test_bag.mcap
Bag size:          2.6 GiB
Storage id:        mcap
ROS Distro:        unknown
Duration:          39.541s
Start:             Jun 19 2024 23:40:38.345 (1718833238.345)
End:               Jun 19 2024 23:41:17.886 (1718833277.886)
Messages:          2310
Topic information: Topic: /parameter_events | Type: rcl_interfaces/msg/ParameterEvent | Count: 0 | Serialization Format: cdr
                   Topic: /velodyne_points | Type: sensor_msgs/msg/PointCloud2 | Count: 765 | Size Contribution: 557.0 MiB | Serialization Format: cdr
                   Topic: /events/read_split | Type: rosbag2_interfaces/msg/ReadSplitEvent | Count: 0 | Serialization Format: cdr
                   Topic: /image_raw_1 | Type: sensor_msgs/msg/Image | Count: 763 | Size Contribution: 1.0 GiB | Serialization Format: cdr
                   Topic: /image_raw_2 | Type: sensor_msgs/msg/Image | Count: 763 | Size Contribution: 1.0 GiB | Serialization Format: cdr
                   Topic: /events/write_split | Type: rosbag2_interfaces/msg/WriteSplitEvent | Count: 0 | Serialization Format: cdr
                   Topic: /rosout | Type: rcl_interfaces/msg/Log | Count: 19 | Size Contribution: 4.9 KiB | Serialization Format: cdr
Service:           0
Service information:

The total size contribution of each topic is computed by sequentially reading every message in the rosbag. As mentioned in #1601, this approach can be slow.

In a first test A (previous output), the computation of the size contribution took ~520ms for a bag with 2310 messages, 2.6GiB large, 39.5s long, with two sensor_msgs/msg/Image topics with 763 messages each, and one sensor_msgs/msg/PointCloud2 topic with 765 messages.

In a second test B, the size computation took ~78ms for a bag with 14577 messages, 5.2MiB large, 74.4s long, with one sensor_msgs/msg/Imu topic with 14559 messages.

So to me, it seems the computation time is not simply dependent on the number of messages, but on their types and individual sizes too. As a test, I was curious about skipping the message serialization step inside the reader. I tried a brute modification of the mcap_storage interface to directly access the messageView.message.dataSize variable while skipping the message serialization step. However, this only resulted in a 15-30% improvement in computation time.

As a test, removing the actual size computation code and just reading the rosbag in an empty while loop doesn't really change the timing (only real difference was in test B, 78ms->68ms).

In rosbag2_py/_info.cpp, the new function compute_topics_size_contribution can be combined with the existing read_service_info to avoid reading the rosbag twice.

@MichaelOrlov looking forward to your feedback and suggested changes.

@nicolaloi nicolaloi requested a review from a team as a code owner June 25, 2024 21:34
@nicolaloi nicolaloi requested review from emersonknapp and james-rms and removed request for a team June 25, 2024 21:34
@MichaelOrlov MichaelOrlov self-requested a review June 25, 2024 23:14
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.

@nicolaloi Thanks for your contribution.
Without deserialization, the message's size calculation per topic will be pretty fast in the vast majority of cases. Perhaps no more than a few seconds. I think adding a separate CLI argument like --size-contribution doesn't make sense. It will be much simpler to make output for topics size contribution by default for the --verbose CLI option.

See my detailed comments and suggestions inline.

rosbag2_cpp/src/rosbag2_cpp/info.cpp Outdated Show resolved Hide resolved
ros2bag/ros2bag/verb/info.py Outdated Show resolved Hide resolved
ros2bag/ros2bag/verb/info.py Outdated Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/format_bag_metadata.cpp Outdated Show resolved Hide resolved
@MichaelOrlov MichaelOrlov marked this pull request as draft June 26, 2024 18:21
@nicolaloi nicolaloi marked this pull request as ready for review June 30, 2024 14:24
@nicolaloi nicolaloi marked this pull request as draft June 30, 2024 14:50
@nicolaloi nicolaloi force-pushed the nicolaloi/size-contribution branch from 586e4fb to a723f75 Compare June 30, 2024 16:30
@nicolaloi nicolaloi marked this pull request as ready for review June 30, 2024 16:32
@nicolaloi
Copy link
Contributor Author

Addressed review comments, added size contribution for services, and updated tests and design doc.

@nicolaloi nicolaloi changed the title Add optional computation of size contribution to info verb Add computation of size contribution to info verb Jul 2, 2024
@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@nicolaloi
Copy link
Contributor Author

I will edit the file with the style divergence error. I have missed it since locally the tests no longer gave me any errors.

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.

@nicolaloi Thanks for addressing the review comments.
Overall, it looks good among a few nitpicks, mainly related to using const references.

rosbag2_py/src/rosbag2_py/_info.cpp Outdated Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/_info.cpp Outdated Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/_info.cpp Outdated Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/_info.cpp Outdated Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/_info.cpp Outdated Show resolved Hide resolved
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.

Thanks looks good to me if CI will pass green.

@MichaelOrlov
Copy link
Contributor

@nicolaloi The Rpr job failed because need to make rebase.
Please rebase your branch on top of latest rolling branch.

@nicolaloi nicolaloi force-pushed the nicolaloi/size-contribution branch 2 times, most recently from e3e5f8d to b131440 Compare July 22, 2024 08:50
nicolaloi and others added 7 commits July 22, 2024 12:13
Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Nicola Loi <[email protected]>
- Also update rosbag2_tests

Signed-off-by: Nicola Loi <[email protected]>
- Also add new test and update design doc

Signed-off-by: Nicola Loi <[email protected]>
Signed-off-by: Nicola Loi <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Nicola Loi <[email protected]>
@nicolaloi nicolaloi force-pushed the nicolaloi/size-contribution branch from b131440 to 1a52858 Compare July 22, 2024 10:13
@nicolaloi
Copy link
Contributor Author

nicolaloi commented Jul 22, 2024

@MichaelOrlov I have rebased the branch and I have updated the new size contribution test I have added to match the latest changes in rolling, but now the Rpr test is failing due to an unrelated rcpputils::fs issue, as in the latest rolling merged PR #1740 (log L32773)

@MichaelOrlov
Copy link
Contributor

@nicolaloi No worries. Those test failures are unrelated to the changes from this PR and test failing because the CI job from GitHub actions uses binaries for the other dependent packages from the latest release on rolling.
The test failure will go away when we will release a new version of the rcpputils package and do rolling synch.

Anyway, I will run CI jobs on the ROS 2 build farm, which compiles all packages from sources. If everything is correct, all tests should pass.

@MichaelOrlov
Copy link
Contributor

Pulls: #1726
Gist: https://gist.githubusercontent.com/MichaelOrlov/5179a9d2b5bc8685c059758e5d9397de/raw/af0dc6b9cdc1954a535c28a5eeb3e5f43c031848/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14281

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

@MichaelOrlov MichaelOrlov merged commit 22148a7 into ros2:rolling Jul 23, 2024
13 of 14 checks passed
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