Skip to content

Conversation

@peci1
Copy link
Contributor

@peci1 peci1 commented May 8, 2025

There were two problems in the codebase:

  1. timeline._update_index_cache was called per invalidated topic, which resulted in multiple reads of the whole bagfile (each topic seeks the reader to the bag start and forces it to read throughout the whole bag). There might be some caching on the way so that rqt_bag doesn't read 20x12 GB for a 12 GB file with 20 topics, but it did definitely read much more than 12 GB.
  2. Once I fixed the above to read all topics at once, another bug popped up: the bag entries were cached in memory so that they could be sorted before yielding them from BagTimeline.get_entries(). This was a smaller problem in the per-topic scenario, but a big problem in the all-topic one. But even in the per-topic scenario, rqt_bag needlessly cached gigabytes of entries (e.g. for an image topic) just to sort them.

This PR fixes both issues by utilizing a set of generators, one per bagfile, which produce messages simultaneousely and the earliest of all messages is taken.

I also got the progressbar working much more nicely (however, it only seems to work when loading the first bag).

Tests

The tests were performed on a 12 GB MCAP bag. Before each test, the bag file was evicted from FS cache.

I also wanted to test on a 70 GB bag file, but I didn't have enough hours and RAM to wait until it loads from a USB3 SSD. With this PR, it should be easy to open it (however, I can only verify that tomorrow).

Without this PR

real    1m7.152s
user    0m25.110s
sys     0m32.818s
Obrazovkove.vysilani.2025-05-08.13.49.15.mp4

With this PR

real    0m33.936s
user    0m16.651s
sys     0m7.837s
Obrazovkove.vysilani.2025-05-08.13.47.10.mp4

The problem mentioned in #166 that clicking on the timeline is super slow on large bags, still remains, however. But that's for a future PR.

Opening a bag now does not read it multiple times and the entries are not buffered in memory.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@peci1
Copy link
Contributor Author

peci1 commented May 8, 2025

Jazzy backport with minimal changes is here: peci1@d90624d .

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

There are some conflicts, do you mind to take a look ?

@peci1
Copy link
Contributor Author

peci1 commented Jul 25, 2025

I'll have a look.

# Conflicts:
#	rqt_bag/src/rqt_bag/rosbag2.py
@peci1
Copy link
Contributor Author

peci1 commented Jul 25, 2025

Updated.

@ahcorde
Copy link
Contributor

ahcorde commented Jul 25, 2025

Pulls: #178
Gist: https://gist.githubusercontent.com/ahcorde/0bf04d7e069601bf739eed5f1af18b1a/raw/2e1907372dd784c1e08e30a5c9ca72645c3e8626/ros2.repos
BUILD args: --packages-above-and-dependencies rqt_bag
TEST args: --packages-above rqt_bag
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16610

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

@ahcorde ahcorde merged commit c7d3efd into ros-visualization:rolling Jul 25, 2025
1 check passed
@peci1
Copy link
Contributor Author

peci1 commented Jul 25, 2025

This PR should not break API (it only adds optional keyword args to a few methods and a few new methods).

The behavior of all existing functions should also be the same (except they might be much faster).

So I think this PR can be backported to Kilted and Jazzy.

@peci1
Copy link
Contributor Author

peci1 commented Jul 25, 2025

The only behavioral change is that get_entries_with_bags() from bag_timeline.py started to respect the topics argument. The previous implementation ignored it (which seemed more a bug than a feature).

@ahcorde
Copy link
Contributor

ahcorde commented Jul 28, 2025

https://github.com/Mergifyio backport kilted jazzy

@mergify
Copy link

mergify bot commented Jul 28, 2025

backport kilted jazzy

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Jul 28, 2025
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
(cherry picked from commit c7d3efd)

# Conflicts:
#	rqt_bag/src/rqt_bag/bag_timeline.py
#	rqt_bag/src/rqt_bag/rosbag2.py
#	rqt_bag/src/rqt_bag/timeline_frame.py
mergify bot pushed a commit that referenced this pull request Jul 28, 2025
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
(cherry picked from commit c7d3efd)
ahcorde pushed a commit that referenced this pull request Jul 28, 2025
Better handling of large bag files (#178)


(cherry picked from commit c7d3efd)

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
Co-authored-by: Martin Pecka <peckama2@fel.cvut.cz>
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