Circular Message Cache implementation for snapshot feature #844

camm73 · 2021-08-11T22:02:19Z

Part of #663

Implements the first part of the new snapshot feature by creating a MessageCacheCircularBuffer class which implements a circular buffer used to store messages until the buffers are flipped. This class is used by CircularMessageCache to create two circular buffers where the primary buffer is continuously filled with messages, overwriting old messages when full, until a buffer swap is triggered. Once the swap occurs, the recorded messages for the snapshot can now be accessed.

MichaelOrlov

@camm73 Thank you for your contribution.
Implementation of MessageCacheCircularBufer with linked list looks very reasonable, however I would recommend to keep the same interface as for MessageCacheBuffer i.e. will need to change return type for MessageCacheCircularBuffer::data() from list to the vector.
Inside MessageCacheCircularBuffer::data() you can create new std::vector<buffer_element_t> for returning value and push elements in it from your internal buffer_ which is linked list.
We are operating with shared_pointers to the messages in linked list and vector and copying elements will not cause a significant overhead since it will be copied only a few bytes for each shared pointer. And whole purpose of the circular buffer is to get data occasionally by request. There are no expectations that request for data from circular buffer will occur in tight loop.

Adhere to the common interface with MessageCacheBuffer will allow to better design structure of the classes. It will be more reasonable to create common pure virtual interface class or base class for message cache buffers.

rosbag2_cpp/include/rosbag2_cpp/cache/circular_message_cache.hpp

rosbag2_cpp/test/rosbag2_cpp/test_circular_message_cache.cpp

rosbag2_cpp/src/rosbag2_cpp/cache/message_cache_circular_buffer.cpp

rosbag2_cpp/include/rosbag2_cpp/cache/message_cache_circular_buffer.hpp

emersonknapp · 2021-08-31T17:33:26Z

Sorry to put this out there so late - but did you see https://github.com/cameron314/readerwriterqueue/blob/master/readerwritercircularbuffer.h - that the same lock-free queue library we were using has a circular implementation? It would be worth trying this out, as it's probably much more tuned and performant than anything we could build ourselves.

EDIT: I just remembered that we are currently only using the shared_queues_vendor/moodycamel queue in the Player, not in the Recorder flow right now. However, maybe it's worth taking a look at regardless - the fewer advanced data structures we have to maintain/bugfix/performance-tune, the better.

emersonknapp

See my comment about the linkedlist implementation, and about the Cache type hierarchy. We might be better off using somebody else's implementation rather than creating our own (though it is a fun exercise to do) - but if we do build it ourselves, a vector should provide much better performance than a linked list.

rosbag2_cpp/include/rosbag2_cpp/cache/circular_message_cache.hpp

rosbag2_cpp/src/rosbag2_cpp/cache/message_cache_circular_buffer.cpp

rosbag2_cpp/include/rosbag2_cpp/cache/message_cache_circular_buffer.hpp

rosbag2_cpp/src/rosbag2_cpp/cache/message_cache_circular_buffer.cpp

MichaelOrlov

It's certainly getting better, however I would like to request several changes, mostly a nitpicks but still very important:

Please remove Base prefix from newly defined interfaces BaseMessageCacheInterface and BaseCacheBufferInterface it's really interfaces without any implementations in it.
Word base is extra in this case and introduce confusion.
Please move interfaces out of the cache_interfaces and put them inside rosbag2_cpp/cache/ folder and namespace, for the sake of the simplification and avoiding long names in the code.
See my other comments in code.

rosbag2_cpp/include/rosbag2_cpp/cache/message_cache.hpp

rosbag2_cpp/include/rosbag2_cpp/cache_interfaces/base_cache_buffer_interface.hpp

rosbag2_cpp/include/rosbag2_cpp/cache/message_cache_buffer.hpp

rosbag2_cpp/include/rosbag2_cpp/cache/message_cache_circular_buffer.hpp

rosbag2_cpp/include/rosbag2_cpp/cache_interfaces/base_message_cache_interface.hpp

rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp

rosbag2_cpp/src/rosbag2_cpp/cache/cache_consumer.cpp

rosbag2_cpp/src/rosbag2_cpp/cache/circular_message_cache.cpp

rosbag2_cpp/src/rosbag2_cpp/cache/message_cache.cpp

camm73 · 2021-09-08T19:10:14Z

Thanks for the comments @MichaelOrlov. Just pushed changes for those

emersonknapp

Looking great - just asking for a quick namespace and docstring update then I think we're good to go

rosbag2_cpp/include/rosbag2_cpp/cache/cache_buffer_interface.hpp

MichaelOrlov

LGTM

rosbag2_cpp/include/rosbag2_cpp/cache/circular_message_cache.hpp

emersonknapp · 2021-09-10T20:55:44Z

Linux
Linux-aarch64
macOS
Windows

emersonknapp · 2021-09-10T22:26:30Z

Oof just a couple of compiler warnings - so close! I'll try and push a commit fixing those, since I think @camm73 may not be reachable for a few days

Signed-off-by: Cameron Miller <[email protected]>

…64_t for buffer sizes Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2021-09-11T00:48:37Z

Rebased and definitely fixed Clang warnings - took a best guess at Windows, it seems like something changed underneath us for create_subscription, the warning came from a package untouched in this PR. In the interest of going green, touched that file in rosbag2_transport

Linux
Linux-aarch64
macOS
Windows

emersonknapp · 2021-09-13T19:06:27Z

🤞

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Emerson Knapp <[email protected]>

* Add circular message cache buffer implementation to be used for in-memory snapshot mode Signed-off-by: Cameron Miller <[email protected]> Co-authored-by: Emerson Knapp <[email protected]> Signed-off-by: 兰陈昕 <[email protected]>

* Add circular message cache buffer implementation to be used for in-memory snapshot mode Signed-off-by: Cameron Miller <[email protected]> Co-authored-by: Emerson Knapp <[email protected]>

camm73 requested a review from a team as a code owner August 11, 2021 22:02

camm73 requested review from lihui815 and david-prody and removed request for a team August 11, 2021 22:02

emersonknapp self-requested a review August 12, 2021 02:38

This was referenced Aug 18, 2021

Implement snapshot mechanism and corresponding ROS Service #850

Merged

Add --snapshot-mode argument to the "record" verb #851

Merged

MichaelOrlov requested changes Aug 28, 2021

View reviewed changes

MichaelOrlov reviewed Aug 28, 2021

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/cache/message_cache_circular_buffer.hpp Outdated Show resolved Hide resolved

emersonknapp requested changes Aug 31, 2021

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/cache/circular_message_cache.hpp Outdated Show resolved Hide resolved

rosbag2_cpp/src/rosbag2_cpp/cache/message_cache_circular_buffer.cpp Outdated Show resolved Hide resolved

emersonknapp reviewed Sep 1, 2021

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/cache/message_cache_circular_buffer.hpp Outdated Show resolved Hide resolved

rosbag2_cpp/src/rosbag2_cpp/cache/message_cache_circular_buffer.cpp Outdated Show resolved Hide resolved

camm73 requested a review from emersonknapp September 7, 2021 22:39

MichaelOrlov requested changes Sep 8, 2021

View reviewed changes

camm73 force-pushed the cameron/circular-buffer-message-cache branch from 95ab166 to 5fecccc Compare September 8, 2021 19:07

emersonknapp requested changes Sep 9, 2021

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/cache/cache_buffer_interface.hpp Outdated Show resolved Hide resolved

rosbag2_cpp/include/rosbag2_cpp/cache/cache_buffer_interface.hpp Show resolved Hide resolved

MichaelOrlov approved these changes Sep 10, 2021

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/cache/circular_message_cache.hpp Outdated Show resolved Hide resolved

emersonknapp approved these changes Sep 10, 2021

View reviewed changes

Cameron Miller added 9 commits September 10, 2021 15:56

Add circular message cache buffer implementation

3616f46

Signed-off-by: Cameron Miller <[email protected]>

Simplified interfaces and updated Cmake

21a5041

Signed-off-by: Cameron Miller <[email protected]>

Add more testing for circular buffer

5c7a1eb

Signed-off-by: Cameron Miller <[email protected]>

Clean up implementation

f0ba9d4

Signed-off-by: Cameron Miller <[email protected]>

Renamed circular buffer class

83bb9aa

Signed-off-by: Cameron Miller <[email protected]>

Convert circular buffer to deque; move documentation

ff39bad

Signed-off-by: Cameron Miller <[email protected]>

Delete default constructor for MessageCacheCircularBuffer

3c3c324

Signed-off-by: Cameron Miller <[email protected]>

Remove unused variables and simplify tests

b12934e

Signed-off-by: Cameron Miller <[email protected]>

Fix MessageCacheCircularBuffer data() return type

ffdbf05

Signed-off-by: Cameron Miller <[email protected]>

Cameron Miller added 7 commits September 10, 2021 15:56

Prevent messages from exceeding circular buffer limit

f32fdd1

Signed-off-by: Cameron Miller <[email protected]>

Improve message copying performance for circular buffer data()

5d230cf

Signed-off-by: Cameron Miller <[email protected]>

Add shared interfaces for messages caches and underlying buffers

ce7d114

Signed-off-by: Cameron Miller <[email protected]>

Switch MessageCache objects to use base interface

d60299a

Signed-off-by: Cameron Miller <[email protected]>

Make notify_buffer_consumer a private function

09e8472

Signed-off-by: Cameron Miller <[email protected]>

Restructure cache interfaces and cleanup cache implementation

ec83a1a

Signed-off-by: Cameron Miller <[email protected]>

Update docstrings; move buffer_element_t declaration

513e1be

Signed-off-by: Cameron Miller <[email protected]>

emersonknapp force-pushed the cameron/circular-buffer-message-cache branch from 88a128b to d24c327 Compare September 11, 2021 00:41

Fix clang and msbuild warnings - choose to use size_t instead of uint…

ccbf1f9

…64_t for buffer sizes Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp force-pushed the cameron/circular-buffer-message-cache branch from d24c327 to ccbf1f9 Compare September 11, 2021 00:46

emersonknapp force-pushed the cameron/circular-buffer-message-cache branch from a600d06 to ea68b1a Compare September 13, 2021 19:04

Fix size_t warning windows

9390c8d

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp force-pushed the cameron/circular-buffer-message-cache branch from ea68b1a to 9390c8d Compare September 13, 2021 19:53

emersonknapp merged commit 622b0ad into ros2:master Sep 13, 2021

vachelyang mentioned this pull request Jun 3, 2022

How to porting or upgrade the snapshot feature to ROS2 galactic. #1023

Closed

YXL76 mentioned this pull request Nov 18, 2022

[backport galactic] snapshot feature (backport #797 #804 #837 #844 #850 #851 #877 #880) #1168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Circular Message Cache implementation for snapshot feature #844

Circular Message Cache implementation for snapshot feature #844

camm73 commented Aug 11, 2021 •

edited by emersonknapp

Loading

MichaelOrlov left a comment

emersonknapp commented Aug 31, 2021 •

edited

Loading

emersonknapp left a comment

MichaelOrlov left a comment

camm73 commented Sep 8, 2021

emersonknapp left a comment

MichaelOrlov left a comment

emersonknapp commented Sep 10, 2021

emersonknapp commented Sep 10, 2021

emersonknapp commented Sep 11, 2021

emersonknapp commented Sep 13, 2021 •

edited

Loading

Circular Message Cache implementation for snapshot feature #844

Circular Message Cache implementation for snapshot feature #844

Conversation

camm73 commented Aug 11, 2021 • edited by emersonknapp Loading

MichaelOrlov left a comment

Choose a reason for hiding this comment

emersonknapp commented Aug 31, 2021 • edited Loading

emersonknapp left a comment

Choose a reason for hiding this comment

MichaelOrlov left a comment

Choose a reason for hiding this comment

camm73 commented Sep 8, 2021

emersonknapp left a comment

Choose a reason for hiding this comment

MichaelOrlov left a comment

Choose a reason for hiding this comment

emersonknapp commented Sep 10, 2021

emersonknapp commented Sep 10, 2021

emersonknapp commented Sep 11, 2021

emersonknapp commented Sep 13, 2021 • edited Loading

camm73 commented Aug 11, 2021 •

edited by emersonknapp

Loading

emersonknapp commented Aug 31, 2021 •

edited

Loading

emersonknapp commented Sep 13, 2021 •

edited

Loading