Sqlite storage double buffering

[adamdbrw]

Ongoing work on implementation for #436

[adamdbrw]

Current issues:

• Role of max_cache_size parameter is confused atm. The vector storing messages should likely be removed now.
• Decision whether buffering is storage implementation specific -> in the abstraction
• When to open transaction. Also, transaction size/cache size should not be specified by number of messages (this is rather poor way to parametrize for the user)

[emersonknapp]

Note that cache size is now in bytes rather than number of messages, as of #530 - max_cache_size should probably be equivalent to the transaction size to start, unless you think they should be different?

[pijaro]

Yes, we are aware that max_cache_size is now a size in bytes. Currently we use max_cache_size as a limit for each buffer (proposed 2 buffers configuration) whereas transaction size is dynamic but <= max_cache_size, depending on disk write speed (if the disk is throttled with writing data, then transaction_size ~= max_cache_size).

We will update draft soon.

[Karsten1987]

Looks good to me. Let' see what CI has to say:

• Linux
• Linux-aarch64
• macOS
• Windows

[adamdbrw]

Quite some warnings on Windows, but pretty straightforward to address. We will handle them tomorrow.

[adamdbrw]

@Karsten1987 @mjeronimo, could you re-trigger Windows CI job? Otherwise this is ready to merge as far as I am concerned.

Also, could you allow @pijaro to push branches to rosbag2? Would make it a bit easier for us.

[mjeronimo]

Restarting Windows build

@adamdbrw - @Karsten1987 will have to add @pijaro; I'm not an owner of this repo and can't grant permissions.

[adamdbrw]

There are 9 warnings left. They don't look like they are associated with the PR changes though.. or am I wrong?

[Karsten1987]

The same windows warnings are currently seen on the nightlies. So yes, there are unrelated.

@mjeronimo I'll leave it up to you to take a pass on this as well.

[mjeronimo]

@adamdbrw I've also confirmed that all of the CMakeLists.txt warnings are also in the nightlies. However, there is one new warning related to the use of the std::accumulate template function from the <numeric> header:

'=': conversion from 'size_t' to '_Ty', possible loss of data

Which, I believe, is referring to the usage here:

https://github.com/ros2/rosbag2/blob/sqlite_storage_double_buffering/rosbag2_cpp/test/rosbag2_cpp/test_message_cache.cpp#L54

The sizes of size_t and uint32_t are different (size_t == 8 bytes, while uint32_t == 4 bytes). I think you can get rid of this one with some slight modifications to the code to use the appropriate types for the accumulation.

[adamdbrw]

@mjeronimo: final warning should be fixed now

[adamdbrw]

Performance of master vs this PR:

Note that both old and new default for cache (--max-cache-size parameter), namely 1MB and 100MB, are included in results. For this test setup, with 100MB to 500MB of messages being written each second, a cache of 1MB is pathological. Note that Recorded Messages % reaches 100% for each row when this PR is combined with #568.

[mjeronimo]

Restarting Windows build to confirm removal of warning:

[adamdbrw]

Seems like the numeric warning is gone and only cmake (unrelated) ones remain. Are we good to merge this?

[mjeronimo]

Yes, just confirmed with the latest build that all CMake warnings are also in the nightly build and that the warning is gone. Merging.

[emersonknapp]

Woohoo! Thanks everyone for pushing this through - it looks great, this is a huge improvement for rosbag2.

[y-okumura-isp]

Are there any plans to backport it to Foxy?

[emersonknapp]

Are there any plans to backport it to Foxy?

It's not certain yet - the code has diverged significantly in API-breaking ways from the Foxy release that will make it hard to backport this on its own. We're having an active conversation now to potentially backport all of the latest rosbag2 code to Foxy, but this is in its early stages.