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

Dnae adas/sequential writing #88

Closed
wants to merge 8 commits into from

Conversation

DensoADAS
Copy link
Contributor

Feature: Writing serialized ROS2 messages to a binary file instead of sqlite3 database.

  • speed improvement
  • allows to split big recordings
  • allows to write to multiple disks (for higher write performance)

Question: Should the identifier for the storage plugin be changed? (rosbag2 recorded with this system cannot be played by previous versions, but vice versa)

Copy link
Collaborator

@Karsten1987 Karsten1987 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 still a few nitpicks, but I request changes mainly for more concrete tests.

@@ -19,6 +19,8 @@
#include <string>
#include <unordered_map>
#include <vector>
#include <fstream>
Copy link
Collaborator

Choose a reason for hiding this comment

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

please order the includes in alphabetical order

std::make_unique<BinarySequentialFile>(database_name_ + ".extra")
);
}
else if(metadata->relative_file_paths.size()>=1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

spaces around the operators

Copy link
Collaborator

Choose a reason for hiding this comment

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

how does such a metafile now look like? What's the enforced format for the relative file paths ?

file_handle->file_stream.write(reinterpret_cast<const char *>(message->serialized_data->buffer),
message->serialized_data->buffer_length);

auto serialized_data = std::make_shared<rcutils_uint8_array_t>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am having trouble to understand what's happening with this serialized_data instance.

@Karsten1987
Copy link
Collaborator

I have a fundamental question for this PR.
What's the intended usage of this extra file? The way I see this every incoming message will be stored twice now. Once in the original .db3 and additionally in the db3.extra file.
I would actually recommend having this as a new and separate storage plugin with a new identifier. This further than allows to stay backwards compatible to existing rosbags but also makes the distinguishing between sqlite and native file clear.

@DensoADAS
Copy link
Contributor Author

Separated the plugin into "sqlite3_sequential". Main difference between both plugins are:
a) in the original version the content of the messages are stored in the database itself (the CPU encodes the message; the database gets quite big for images or similar content)
b) while in the sequential-version
- the content of the message is stored in one (or more) binary files (which can be written without encoding)
- in the database itself, only the offset (uint64) and the size of the message (uint64) and the file index (corresponding to the relative file paths in the metafile) are stored

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request Feb 28, 2019
* ros2GH-69 Read storage content in a separate thread

For now the publishing starts only after the reading is completly
done. This should change aufter ros2GH-68 is done and a thread-safe
queue can be used instead of std::queue.

* ros2GH-71 Add integration test for timing behavior

* ros2GH-68 Introduce vendor package for shared queue

- Download and install headers from moodycamel readerwriterqueue
- Download and install headers from moodycamel concurrentqueue
- Use readerwriterqueue in code to load and publish concurrently

* ros2GH-71 Retain time difference of messages when playing a bag file

- The main (play) thread sleeps until the time for publishing the
  message is reached.
- Using std::chrono time_point and duration for type-safe time
  arithmetic instead of rcutils time types.

* ros2GH-71 Improve stability of read test

- Subscribers need to maintain a longer history if the messages are
  not consumed fast enough.

* ros2GH-71 Fix Classloader instance lifetime

The Classloader instance needs to outlive all objects created by it.

* ros2GH-71 Extract playing code into a class of its own

Reason: record and play have almost no common code but do the exact
opposite with the storage and rclcpp.

* ros2GH-70 Do not link explicitly against std_msgs

- only required in tests
- this decreases the amount of packages needed for a clean build without tests

* ros2GH-70 Fix error message of storage

* ros2GH-70 Fix pluginlib/storage issue for recording

* ros2GH-71 Cleanup: variable naming

* ros2GH-70 Load storage continuously instead of as fast as possible

- Only load if queue contains less than 1000 messages
- Wait a millisecond before loading again once the queue is long enough

* ros2GH-70 Add options struct to allow specification of queue size

* ros2GH-72 Wait for messages to fill up

* ros2GH-74 Rename integration tests to play/record tests

* ros2GH-74 Use test_msgs in integration tests

- gets rid of string_msgs dependency

* ros2GH-70 Rename is_not_ready to is_pending, use bulk reading to queue

* ros2GH-70 Harmonize storage_loading_future variable

* ros2GH-88 Read messages in order of their timestamps

- Currently, we write sequentially in order of arrival time so
  reading in id order is fine
- This may change at a later time and should not change the reading
  behaviour, i.e. we need to read in order of timestamps

* Fix compiler error on Mac

* ros2GH-8 Fix: use correct ros message type in test

* ros2GH-8 Cleanup: minor code style fixes

* ros2GH-8 Refactor future usage in player

Make the future a class member of player to avoid having to hand it
into several functions which is difficult with a move-only type.

* ros2GH-8 Cleanup: remove verbose logging for every stored message

* ros2GH-8 Refactor rosbag2 interface

Add an explicit overload for record without a topic_names argument to
record all topics.

* fix: call vector.reserve instead of default initalization

* fix record demo
Joshua Hampp added 8 commits August 12, 2019 15:14
… (for sqlite3):

 * storing actual message data into a binary file (sequentially)
 * storing metainformation (offset and length of message content) in database
 * perfomance improvement

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
… allocater for rclutils_init...)

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
 * implemented support for multiple files (could be used to split up big data or write on multiple disks)
 * support multithreading

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
 * updated test cases to extra files

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
 * sqlite3 is the original ROS2 version
 * sqlite3_sequential is storing the actual data in separate file(s) which are writen binary and sequential
 * added get_storage_identifier() to throw an exception if wrong storage plugin was loaded

Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
@DensoADAS
Copy link
Contributor Author

  • changes due to request
  • rebased

@Karsten1987
Copy link
Collaborator

@DensoADAS sorry for stalling this PR for so long. Do you mind rebasing this PR once more? Or is that no longer relevant? I remember there was some connection to the newly introduced intra process communication.

@DensoADAS
Copy link
Contributor Author

DensoADAS commented Feb 13, 2020

@Karsten1987
We're waiting for rclpp PR #291 973
We prepared an updated version of rosbag2 compatible to these changes.

@emersonknapp emersonknapp added this to Needs Review in Rosbag2 Galactic Dec 7, 2020
@emersonknapp
Copy link
Collaborator

@DensoADAS are you still planning on implementing this feature? The code has diverged considerably since this was opened, so I imagine it would need a bit of rework.

It also seems like this could be implemented in a separate package as a standalone plugin, rather than going into rosbag2_storage_default_plugins. I'm inclined to only install a single storage plugin by default, allowing other storage implementations to be optionally installed. If so, that would mean this should definitely be its own package, and I'm inclined to think, its own repository as well.

I'm going to close this for now, and if you would like to continue the conversation/development about including it in this repository, please feel free to reopen.

@emersonknapp emersonknapp removed this from Needs Review in Rosbag2 Galactic Apr 23, 2021
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.

None yet

4 participants