From 21b2c80acb4ce43a8da6f0854154cf11f12dc31e Mon Sep 17 00:00:00 2001 From: Karsten Knese Date: Tue, 31 Mar 2020 17:08:25 -0700 Subject: [PATCH] address review comments Signed-off-by: Karsten Knese --- .../include/rosbag2_cpp/storage_options.hpp | 6 ++-- .../rosbag2_cpp/writers/sequential_writer.hpp | 10 +++---- .../rosbag2_cpp/writers/sequential_writer.cpp | 28 +++++++++---------- .../rosbag2_cpp/test_sequential_writer.cpp | 12 ++++---- 4 files changed, 27 insertions(+), 29 deletions(-) diff --git a/rosbag2_cpp/include/rosbag2_cpp/storage_options.hpp b/rosbag2_cpp/include/rosbag2_cpp/storage_options.hpp index 7adf4342d0..86e97e7177 100644 --- a/rosbag2_cpp/include/rosbag2_cpp/storage_options.hpp +++ b/rosbag2_cpp/include/rosbag2_cpp/storage_options.hpp @@ -30,10 +30,10 @@ struct StorageOptions // A value of 0 indicates that bagfile splitting will not be used. uint64_t max_bagfile_size; - // The chunk size indiciates how many messages should be hold in cache + // The cache size indiciates how many messages can maximally be hold in cache // before these being written to disk. - // Defaults to 1, and effectively disables the caching. - size_t chunk_size = 1; + // Defaults to 0, and effectively disables the caching. + uint64_t max_cache_size = 0; }; } // namespace rosbag2_cpp diff --git a/rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp b/rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp index 4cc1ad5606..be866e8ebf 100644 --- a/rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp +++ b/rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp @@ -110,14 +110,14 @@ class ROSBAG2_CPP_PUBLIC SequentialWriter std::unique_ptr metadata_io_; std::unique_ptr converter_; - // Intermediate cache to write multiple messages into the storage. - // chunk size is the amount of messages to hold in storage before writing to disk. - size_t chunk_size_; - std::vector> cache_; - // Used in bagfile splitting; specifies the best-effort maximum sub-section of a bagfile in bytes. uint64_t max_bagfile_size_; + // Intermediate cache to write multiple messages into the storage. + // `max_cache_size` is the amount of messages to hold in storage before writing to disk. + size_t max_cache_size_; + std::vector> cache_; + // Used to track topic -> message count std::unordered_map topics_names_to_info_; diff --git a/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp b/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp index e18c623f09..a012302a03 100644 --- a/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp +++ b/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp @@ -77,11 +77,11 @@ void SequentialWriter::open( const StorageOptions & storage_options, const ConverterOptions & converter_options) { - max_bagfile_size_ = storage_options.max_bagfile_size; base_folder_ = storage_options.uri; - chunk_size_ = storage_options.chunk_size; + max_bagfile_size_ = storage_options.max_bagfile_size; + max_cache_size_ = storage_options.max_cache_size; - cache_.reserve(chunk_size_); + cache_.reserve(max_cache_size_); if (converter_options.output_serialization_format != converter_options.input_serialization_format) @@ -206,19 +206,17 @@ void SequentialWriter::write(std::shared_ptrwrite(converter_ ? converter_->convert(message) : message); - return; - } - - cache_.push_back(converter_ ? converter_->convert(message) : message); - if (cache_.size() >= chunk_size_) { - storage_->write(cache_); - // reset cache - cache_.clear(); - cache_.reserve(chunk_size_); + } else { + cache_.push_back(converter_ ? converter_->convert(message) : message); + if (cache_.size() >= max_cache_size_) { + storage_->write(cache_); + // reset cache + cache_.clear(); + cache_.reserve(max_cache_size_); + } } } diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp index 3358193206..362e100d31 100644 --- a/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp +++ b/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp @@ -258,12 +258,12 @@ TEST_F(SequentialWriterTest, writer_splits_when_storage_bagfile_size_gt_max_bagf TEST_F(SequentialWriterTest, only_write_after_cache_is_full) { const size_t counter = 1000; - const size_t chunk_size = 100; + const size_t max_cache_size = 100; EXPECT_CALL( *storage_, write(An> &>())). - Times(counter / chunk_size); + Times(counter / max_cache_size); EXPECT_CALL( *storage_, write(An>())).Times(0); @@ -278,7 +278,7 @@ TEST_F(SequentialWriterTest, only_write_after_cache_is_full) { message->topic_name = "test_topic"; storage_options_.max_bagfile_size = 0; - storage_options_.chunk_size = chunk_size; + storage_options_.max_cache_size = max_cache_size; writer_->open(storage_options_, {rmw_format, rmw_format}); writer_->create_topic({"test_topic", "test_msgs/BasicTypes", "", ""}); @@ -288,9 +288,9 @@ TEST_F(SequentialWriterTest, only_write_after_cache_is_full) { } } -TEST_F(SequentialWriterTest, do_not_use_cache_if_chunk_size_is_one) { +TEST_F(SequentialWriterTest, do_not_use_cache_if_cache_size_is_zero) { const size_t counter = 1000; - const size_t chunk_size = 1; + const size_t max_cache_size = 0; EXPECT_CALL( *storage_, @@ -310,7 +310,7 @@ TEST_F(SequentialWriterTest, do_not_use_cache_if_chunk_size_is_one) { message->topic_name = "test_topic"; storage_options_.max_bagfile_size = 0; - storage_options_.chunk_size = chunk_size; + storage_options_.max_cache_size = max_cache_size; writer_->open(storage_options_, {rmw_format, rmw_format}); writer_->create_topic({"test_topic", "test_msgs/BasicTypes", "", ""});