ros2 / rosbag2

Apache License 2.0
285 stars 251 forks source link

Performance improvements in Rosbag2 recorder discovery #1825

Closed Rayman closed 1 month ago

Rayman commented 1 month ago

Screenshot from 2024-09-27 15-22-25

MichaelOrlov commented 1 month ago

@Rayman I would say that PR description "Reintroduce Don't warn for unknown types if topics are not selected" and reference to the

Reintroduce the fix from https://github.com/ros2/rosbag2/commit/51a83f4421ff335c2237e6214e9bb7e6ae206a92

Is incorrect in this case. While both PRs improve performance in the discovery, they are completely different.

fujitatomoya commented 1 month ago

@MichaelOrlov @Rayman so this discovery improvement provided originally https://github.com/ros2/rosbag2/pull/1466, kinda rolled back because of https://github.com/ros2/rosbag2/pull/1480, right? just checking if i understand correctly.

CC: @Barry-Xu-2018

Rayman commented 1 month ago

Yes, I'm referencing the commit Move checks for single type and hidden topics to the bottom from that squashed PR 51a83f4421ff335c2237e6214e9bb7e6ae206a92. That commit got rolled back because of #1485. The node graph checks are quite expensive so I'd move them to the bottom.

MichaelOrlov commented 1 month ago

@Rayman To be clear in this PR you are not doing "Move checks for single type and hidden topics to the bottom" - therefore we can't say that this PR reintroducing changes from the https://github.com/ros2/rosbag2/commit/51a83f4421ff335c2237e6214e9bb7e6ae206a92

I've changed PR title and description accordingly.

MichaelOrlov commented 1 month ago

Pulls: ros2/rosbag2#1825 Gist: https://gist.githubusercontent.com/MichaelOrlov/bc40716811cdff501cae1551da0b955c/raw/5d6c90a2f967cc06fbc14d9efb7d1296caf4ed8c/ros2.repos BUILD args: --packages-above-and-dependencies rosbag2_transport TEST args: --packages-above rosbag2_transport ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14653

MichaelOrlov commented 1 month ago

https://github.com/Mergifyio backport jazzy

mergify[bot] commented 1 month ago

backport jazzy

✅ Backports have been created

* [#1827 Performance improvements in Rosbag2 recorder discovery (backport #1825)](https://github.com/ros2/rosbag2/pull/1827) has been created for branch `jazzy`