Use topic_id instead of topic_name in inner rosbag2 buffers and data structures

[MichaelOrlov]

Description

Motivation:

1. Users complain multiple times for instance here Use middleware send and receive timestamps from message_info during recording #1531 (comment) that rosbag2 stores messages in internal buffers in a very inefficient way and that memory footprint for the same data payload could be reduced down to the factor of the 2 or 3 by using topic_id represented as integer type instead of full topic_name represented as a string for each message.
   This is very meaningful when using the snapshot feature and recording on the host HW with limited resources.
2. Another motivating factor for using topic_id instead of the topic_name in internal data structures and buffers is the ability to differentiate different publishers with the same topic name. It might be a situation for instance when multiple publishers exist on the same topic but have different QoS settings. Another use case could be when multiple publishers exist on the same topic but with different versions of the type definitions.
   Currently, we don't support these use cases in the rosbag2. However, if we will differentiate topics by topic id in inner rosbag2 representation on all layers we would be able to add support for the aforementioned use cases in the future.

Related Issues

#1531

Completion Criteria

• rosbag2 using topic_id in integer representation instead of topic_name for each message stored in inner buffers during recording.
• rosbag2 using topic_id in integer representation instead of topic_name for each message stored in inner buffers during playback or bag rewrite.

Implementation Notes / Suggestions

To achieve the completion criteria we would need to make the following structural changes:

1. Will need to store topic_id instead of the topic_name in the
   

   rosbag2/rosbag2_storage/include/rosbag2_storage/serialized_bag_message.hpp

   Lines 27 to 32 in bbbb217

   	struct SerializedBagMessage
   	{
   	std::shared_ptr<rcutils_uint8_array_t> serialized_data;
   	rcutils_time_point_value_t time_stamp;
   	std::string topic_name;
   	};

   
   and store map with topic_id to topic_name and vice versa where it is needed on the rosbag2_cpp and rosbag2_transport layers.

2. To be able to operate with unique topic_id on rosbag2_transport and rosbag2_cpp layers, especially in the cache buffers need to somehow generate this topic_id or get it from somewhere.
   It turns out that sqlite3 and mcap already operate with unique topic id on the storage layer and generate unique topic_id at the moment of creation of a new topic. It will be reasonable to keep current behavior and let storage plugins assign unique topic id during topic creation. We just need to somehow get it from storage on the recording and playback.
   currently, we have the following create_topic interface
   

   rosbag2/rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_write_interface.hpp

   Lines 44 to 46 in bbbb217

   	virtual void create_topic(
   	const TopicMetadata & topic,
   	const rosbag2_storage::MessageDefinition & message_definition) = 0;

   
   One of the options would be to change this API to return the topic id instead of the void as a result of this method. The another option would be to remove const qualifier from the TopicMetadata & topic parameter and add topic_id to the TopicMetadata data structure. To be able to store topic_id to the SerializedBagMessage during recording it might be enough a first option when returning topic_id from the create_topic(..) API. However, to be able to operate with topic_id during playback and bag rewrite we anyway will need to add topic_id to the TopicMetadata.
   Therefore the second option is more preferable. One more point toward the second option with removing the const qualifier from the TopicMetadata parameter is less invasive API changes. Perhaps these changes wouldn't require significant changes on the downstream dependencies. In contrast, if would need to change the return parameter for the create_topic(..) we would need to go to the full cycle of the deprecation of the old API and create a new create_topic(..) API at the same time. Which is less preferable.

3. After adding topic_id to the TopicMetadata data structure to be able to operate with topic_id during playback and map it to the topic_name will need to change implementations of the get_all_topics_and_types() API.
   

   rosbag2/rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_read_interface.hpp

   Line 75 in bbbb217

   virtual std::vector<TopicMetadata> get_all_topics_and_types() = 0;

   
   In all storage plugins to read out from storage and populate this newly added topic_id field.

4. Size of the type of the newly adding topic_id field.
   In MCAP specification we reserved 16 bit for the topic_id. This is the ChannelId the MCAP inner type which corresponds to the uint16_t. When we were creating the MCAP format we decided that the ability to differentiate a maximum 65535 topics would be enough in the vast majority of cases. However, in the SQLite3 storage plugin we are using topic_id as an index to map records from the messages DB table to the corresponding topics table. According to the SQLite3 specification, the index is the int64_t data type and can't be changed.
   Therefore the type of the newly adding topic_id filed shall be no less than int64_t and shall be with the sign.

Testing Notes / Suggestions

The main functionality is expected to be already covered by the existent unit tests, however, a few new tests might be needed.

[MichaelOrlov]

I finally found time to put it all together in one feature request.
cc @@jmachowinski @fujitatomoya @emersonknapp Any thoughts or concerns?

[arneboe]

4. Size of the type of the newly adding topic_id field.
In MCAP specification we reserved 16 bit for the topic_id. This is the ChannelId the MCAP inner type which corresponds to the uint16_t. When we were creating the MCAP format we decided that the ability to differentiate a maximum 65535 topics would be enough in the vast majority of cases. However, in the SQLite3 storage plugin we are using topic_id as an index to map records from the messages DB table to the corresponding topics table. According to the SQLite3 specification, the index is the int64_t data type and can't be changed.
Therefore the type of the newly adding topic_id filed shall be no less than int64_t and shall be with the sign.

Why not keep uint16_t internally and just promote to int64_t inside the sql storage plugin?
The memory overhead of int64_t is significant, especially for small messages.

[MichaelOrlov]

@arneboe This is a good question and proposal.
Well, on one hand, it will require keeping a hash map with two types of IDs in the SQLite3 storage plugin side and having a hash lookup operation for each message read/write. The hash map is not very memory efficient but still could take less memory footprint rather than keeping an extra 2 6 bytes per message. Especially when we are having a lot of high-frequency messages like IMU. Another drawback of this is a little bit of performance hit for lookup for inner ID in the hash map for each message write.
However, on the other hand, we probably can sacrifice a bit of performance penalty on the SQLite3 plugin side for the sake of a better memory footprint.
Please note that MCAP storage plugin will not be affected and users will have benefits using rosbag2 with it.

I will try to make it within the scope of the #1538

[arneboe]

@arneboe This is a good question and proposal. Well, on one hand, it will require keeping a hash map with two types of IDs in the SQLite3 storage plugin side and having a hash lookup operation for each message read/write. The hash map is not very memory efficient but still could take less memory footprint rather than keeping an extra 2 bytes per message. Especially when we are having a lot of high-frequency messages like IMU. Another drawback of this is a little bit of performance hit for lookup for inner ID in the hash map for each message write. However, on the other hand, we probably can sacrifice a bit of performance penalty on the SQLite3 plugin side for the sake of a better memory footprint. Please note that MCAP storage plugin will not be affected and users will have benefits using rosbag2 with it.

I will try to make it within the scope of the #1538

Maybe my understanding of how rosbag works internally is lacking. I don't understand why a hash table is needed.
If the topic_id is used as primary key (i.e. as rowid) it should be possible to just cast the uint16_t to int64_t and use it as key for the first 65535 rows of the table.

In any case that is a micro optimization compared to getting rid of the strings :)
I am perfectly happy if the strings are gone.

[MichaelOrlov]

@arneboe As regards:

it should be possible to just cast the uint16_t to int64_t and use it as key for the first 65535 rows of the table.

It would be possible if we could be sure that the SQLite3 engine will assign indexes sequentially and will not go into the negative part. Although as far as I am aware there is no guarantee for that. Also, please keep in mind that we have the remove_topic(..) API due to that likely indexes might not always grow monotonically.

[fujitatomoya]

after all, i think that is okay to have topic identification in the storage to tell the publishers, but some comments below.

• totally agree on efficiency and the cases for different publishers with the same topic name.
• handling individual publishers would be extra cost to record and play to synchronizing the data from the different record field, also showing the topic information from the database would require some extra process.
• (maybe this is not a main motivation here) do we really need to detect the message drops in rosbag2 after all? I think that is the responsibility for ROS 2 Quality of Service, and user configuration based on the requirements. that said, if QoS is best effort, there would be some message drops, because user sets so? I am not sure if rosbag2 should be able to tell this...

[jmachowinski]

• handling individual publishers would be extra cost to record and play to synchronizing the data from the different record field, also showing the topic information from the database would require some extra process.

I don't think that this will have a huge impact on performance (know, this is my guts speaking, only test will show).

On the other side, I can see positive aspects, as multiple publishes per topic would move the rosbag play nearer to the real system behavior.
E.g. If you have checks that wait for multiple publishers in your node, this would then work out of the box if we move to multiple publishers. (I know is bad design in the first place, but just as an example)

[ros-discourse]

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-meeting-minutes-2024-02-15/36221/1

[MichaelOrlov]

• Relates Play back bag file by node #1169
• Relates Storage channel per-publisher on topic  #1252