SequentialWriter to cache by message size instead of message count #530

jaisontj · 2020-10-02T19:55:36Z

This is part 1 of 2 PRs addressing #464

The SequentialWriter cache now caches by message size and not by message count

As for:

Consider how this might live side-by-side with "cache by time" (maybe "whichever comes first")

It can work the same way as split bag file by duration and split bag file by size in that, it will be "whichever comes first"

rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp

jaisontj · 2020-10-02T20:52:03Z

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

@@ -249,11 +249,13 @@ void SequentialWriter::write(std::shared_ptr<rosbag2_storage::SerializedBagMessa
    storage_->write(converted_msg);


@emersonknapp we discussed cleaning this up better, however, I think this initial 0 check is required cause the cache size is set to 0 in this scenario (cause max_cache_size is 0) and so we need to handle it separately.

Let me know what you think.

jaisontj · 2020-10-02T20:53:42Z

rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp

@@ -314,8 +319,13 @@ TEST_F(SequentialWriterTest, do_not_use_cache_if_cache_size_is_zero) {

  std::string rmw_format = "rmw_format";

+  std::string msg_content = "Hello";


This test ensures that cache is not used if cache size is 0 and previously did not care about the serialized_data in the message. However, I thought it is better if we do test with a close to real world scenario where the message has some data as well.

dabonnie · 2020-10-05T21:17:12Z

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

@@ -117,7 +118,7 @@ TEST_F(RecordFixture, record_end_to_end_test) {
  wrong_message->string_value = "wrong_content";

  auto process_handle = start_execution(
-    "ros2 bag record --output " + root_bag_path_.string() + " /test_topic");
+    "ros2 bag record --max-cache-size 0 --output " + root_bag_path_.string() + " /test_topic");


Isn't 0 the default?

Oops, I should add this to the comment and the title also.

0 default = dont use cache and write to disk always, which isnt great for performance. So as per the issue #464 I've also changed the default to 1MB

ros2bag/ros2bag/verb/record.py

Karsten1987 · 2020-10-06T00:10:31Z

Thanks for picking this up. I haven't had time to fully look into this (will hopefully get to it tomorrow), but as a first glance I just want to challenge the fact that number of messages is not useful. I could see that there's a use case of splitting bag files with a specific message count, e.g. recording a single image topic with the result of having 100 images per bag file.

~~Could we support both? I would see that each condition results in its own predicate which determines when to split. That way we could also easily support splitting by time at some point in the future.~~

jaisontj · 2020-10-06T00:33:13Z

Thanks for picking this up. I haven't had time to fully look into this (will hopefully get to it tomorrow), but as a first glance I just want to challenge the fact that number of messages is not useful. I could see that there's a use case of splitting bag files with a specific message count, e.g. recording a single image topic with the result of having 100 images per bag file.

Could we support both? I would see that each condition results in its own predicate which determines when to split. That way we could also easily support splitting by time at some point in the future.

Hi @Karsten1987, this PR does not change the bag splitting functionality, rather it addresses the way messages are stored in cache before being written to a bag. Previously, it used the number of messages to hold in cache whereas bag splitting happens based on bag size or duration. I didnt see any support for split by message count (I could be wrong though).

Let me know if I've understood you correctly.

Karsten1987 · 2020-10-06T00:45:08Z

I haven't had time to fully look into this

That's what I meant with this 🤭
I am sorry, I really should have looked better at this. Please ignore my comment.

jaisontj · 2020-10-06T01:11:28Z

I haven't had time to fully look into this

That's what I meant with this 🤭
I am sorry, I really should have looked better at this. Please ignore my comment.

Haha, its all good :)

emersonknapp · 2020-10-06T17:22:28Z

Just noting that this touches the same code as #506 for full visibility

dabonnie

Seems reasonable with green CI

emersonknapp

LGTM, nice and minimal!

jaisontj · 2020-10-07T19:54:17Z

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Jaison Titus <jaisontj@amazon.com>

current_cache_size_ Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj · 2020-10-07T21:57:40Z

Looks like there was a change in master which caused the CI to fail. Pushed again after a rebase with master. Running CI again:

Linux
Linux-aarch64
macOS
Windows

jaisontj · 2020-10-08T16:39:24Z

The CI run above tried testing rosbag2_performance_writer_benchmarking which was not built since I used build --packages-up-to rosbag2

Here is the correct one with:
build_args: --packages-up-to rosbag2 ros2bag rosbag2_performance_writer_benchmarking rosbag2_py
test_args: --packages-select-regex rosbag2 ros2bag

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj · 2020-10-09T17:23:08Z

New CI Build after fixing the warning on windows:

build_args: --packages-up-to rosbag2 ros2bag rosbag2_performance_writer_benchmarking rosbag2_py
test_args: --packages-select-regex rosbag2 ros2bag

Linux
Linux-aarch64
macOS
Windows

emersonknapp · 2020-10-09T18:40:22Z

Instability on OSX and aarch64 is related to xmllint, which is failing on all packages currently in the nightly build - so it is definitely not related to this PR

) * Fixes #464 - caches by message size and not message count Signed-off-by: Jaison Titus <jaisontj@amazon.com>

…os2#530) * Fixes ros2#464 - caches by message size and not message count Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj requested a review from a team as a code owner October 2, 2020 19:55

jaisontj requested review from thomas-moulard and Karsten1987 and removed request for a team October 2, 2020 19:55

jaisontj marked this pull request as draft October 2, 2020 19:56

jaisontj requested review from dabonnie, emersonknapp and jikawa-az October 2, 2020 19:57

jaisontj force-pushed the jaisontj/fix-464 branch from c53b93b to 52735bf Compare October 2, 2020 19:59

dabonnie reviewed Oct 2, 2020

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp Outdated Show resolved Hide resolved

jaisontj commented Oct 2, 2020

View reviewed changes

jaisontj changed the title ~~SequentialWriter caches by message size and not message count~~ SequentialWriter to cache by message size and not message count Oct 5, 2020

dabonnie reviewed Oct 5, 2020

View reviewed changes

jaisontj changed the title ~~SequentialWriter to cache by message size and not message count~~ SequentialWriter to cache by message size and change default cache size to 1MB Oct 5, 2020

jaisontj marked this pull request as ready for review October 5, 2020 22:49

dabonnie reviewed Oct 5, 2020

View reviewed changes

ros2bag/ros2bag/verb/record.py Outdated Show resolved Hide resolved

jaisontj force-pushed the jaisontj/fix-464 branch 2 times, most recently from e810f1c to f6e3104 Compare October 7, 2020 05:16

jaisontj changed the title ~~SequentialWriter to cache by message size and change default cache size to 1MB~~ SequentialWriter to cache by message size instead of message count Oct 7, 2020

jaisontj mentioned this pull request Oct 7, 2020

Change default cache size for sequential_writer to a non zero value #533

Merged

dabonnie approved these changes Oct 7, 2020

View reviewed changes

emersonknapp approved these changes Oct 7, 2020

View reviewed changes

Fixes ros2#464 - caches by message size and not message count

6f86080

Signed-off-by: Jaison Titus <jaisontj@amazon.com>

Fixes code style errors and renames current_cache_size to

0fb6aa1

current_cache_size_ Signed-off-by: Jaison Titus <jaisontj@amazon.com>

jaisontj force-pushed the jaisontj/fix-464 branch from f6e3104 to 0fb6aa1 Compare October 7, 2020 21:42

jikawa-az approved these changes Oct 8, 2020

View reviewed changes

Explicitly type cast to int in test to avoid warning in windows.

9e92f58

Signed-off-by: Jaison Titus <jaisontj@amazon.com>

emersonknapp merged commit 8a917af into ros2:master Oct 9, 2020

jaisontj deleted the jaisontj/fix-464 branch October 9, 2020 21:04

emersonknapp mentioned this pull request Oct 29, 2020

Sqlite storage double buffering #546

Merged

emersonknapp pushed a commit that referenced this pull request Feb 2, 2021

SequentialWriter to cache by message size instead of message count (#530

61e85ab

) * Fixes #464 - caches by message size and not message count Signed-off-by: Jaison Titus <jaisontj@amazon.com>

emersonknapp pushed a commit that referenced this pull request Feb 17, 2021

SequentialWriter to cache by message size instead of message count (#530

b124188

) * Fixes #464 - caches by message size and not message count Signed-off-by: Jaison Titus <jaisontj@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SequentialWriter to cache by message size instead of message count #530

SequentialWriter to cache by message size instead of message count #530

jaisontj commented Oct 2, 2020 •

edited

Loading

jaisontj Oct 2, 2020

jaisontj Oct 2, 2020

dabonnie Oct 5, 2020

jaisontj Oct 5, 2020

Karsten1987 commented Oct 6, 2020 •

edited

Loading

jaisontj commented Oct 6, 2020

Karsten1987 commented Oct 6, 2020

jaisontj commented Oct 6, 2020

emersonknapp commented Oct 6, 2020

dabonnie left a comment

emersonknapp left a comment

jaisontj commented Oct 7, 2020

jaisontj commented Oct 7, 2020

jaisontj commented Oct 8, 2020 •

edited

Loading

jaisontj commented Oct 9, 2020

emersonknapp commented Oct 9, 2020

		@@ -249,11 +249,13 @@ void SequentialWriter::write(std::shared_ptr<rosbag2_storage::SerializedBagMessa
		storage_->write(converted_msg);

		@@ -314,8 +319,13 @@ TEST_F(SequentialWriterTest, do_not_use_cache_if_cache_size_is_zero) {

		std::string rmw_format = "rmw_format";

		std::string msg_content = "Hello";

SequentialWriter to cache by message size instead of message count #530

SequentialWriter to cache by message size instead of message count #530

Conversation

jaisontj commented Oct 2, 2020 • edited Loading

jaisontj Oct 2, 2020

Choose a reason for hiding this comment

jaisontj Oct 2, 2020

Choose a reason for hiding this comment

dabonnie Oct 5, 2020

Choose a reason for hiding this comment

jaisontj Oct 5, 2020

Choose a reason for hiding this comment

Karsten1987 commented Oct 6, 2020 • edited Loading

jaisontj commented Oct 6, 2020

Karsten1987 commented Oct 6, 2020

jaisontj commented Oct 6, 2020

emersonknapp commented Oct 6, 2020

dabonnie left a comment

Choose a reason for hiding this comment

emersonknapp left a comment

Choose a reason for hiding this comment

jaisontj commented Oct 7, 2020

jaisontj commented Oct 7, 2020

jaisontj commented Oct 8, 2020 • edited Loading

jaisontj commented Oct 9, 2020

emersonknapp commented Oct 9, 2020

jaisontj commented Oct 2, 2020 •

edited

Loading

Karsten1987 commented Oct 6, 2020 •

edited

Loading

jaisontj commented Oct 8, 2020 •

edited

Loading