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

Check for converter for every message might be performance issue #62

Closed
Karsten1987 opened this issue Nov 23, 2018 · 3 comments
Closed
Labels
enhancement New feature or request help wanted Extra attention is needed more-information-needed Further information is required

Comments

@Karsten1987
Copy link
Collaborator

See here:
https://github.com/ros2/rosbag2/blob/master/rosbag2/src/rosbag2/sequential_reader.cpp#L66

For every message in the bag file there is going to be a check whether or not this is has to be converted. I believe this check can be done once per topic and an appropriate reader shall be used. Whenever the message has to be converted, I believe it makes sense to have a separate class for it which always converts. On the other side, when no conversion is needed (which is most likely almost all time the case), the check should not be issued for every message.

@Karsten1987 Karsten1987 added the enhancement New feature or request label Nov 23, 2018
@anhosi
Copy link
Contributor

anhosi commented Nov 27, 2018

I am not yet convinced that this line really poses a performance issue. Also our rather crude performance tests give no such indication (but they might not be sensitive enough).
More importantly having a single converter will no longer suffice with when #52 is implemented. Then we most likely will have a converter for each topic and every message will have to be dispatched to the converter for the corresponding topic.

We could think about having a "identity converter" for topics that require no conversion. Doing so before #52 is not necessary IMHO.

sriramster pushed a commit to sriramster/rosbag2 that referenced this issue Feb 28, 2019
* ros2GH-61 Read topic directly from message when playing and allow to play multiple topics

* ros2GH-61 Add test for SqliteStorage and update old ones

* ros2GH-62 Extend function to poll for any number of specified topics

* ros2GH-62 Allow subscription to several topics

* ros2GH-61 Obtain the topic name directly from the database

- Uses a JOIN instead of mapping the topic_id to the name in code

* ros2GH-61 Cache read row in result iterator

This allows repeated dereferencing on same row without quering the
database again.

* ros2GH-62 Change demo-record to allow specifying multiple topics

* ros2GH-62 Add test to write non-string topic + refactoring

* ros2GH-62 Add test for subscription to multiple topics

* ros2GH-62 Cleanup

* ros2GH-62 Simplify test setup

* ros2GH-61 Cleanup

* ros2GH-61 consolidate storage integration test

* ros2GH-62 Consolidate write integration tests

* ros2GH-61 enhance read integration test to check multiple topics

* ros2GH-62 Improve rosbag integration test

* ros2GH-62: Polish rosbag2_rosbag_node_test

* ros2GH-62 Fix cpplint

* ros2GH-62 Fix memory leak in rosbag helper

* ros2GH-62 Cleanup of subscriptions

* ros2GH-62 do not use flaky timers in rosbag2_write_integration_test

* ros2GH-62 Use rmw_serialize_message_t consistently in test helper classes

* ros2GH-73 Use test_msgs in read_integration_test

* ros2GH-26 Cleanup: fix alphabetic orderung
@emersonknapp emersonknapp added help wanted Extra attention is needed more-information-needed Further information is required labels Jan 29, 2020
@emersonknapp
Copy link
Collaborator

We need to benchmark this to find out if it's necessary to change

@emersonknapp
Copy link
Collaborator

Best I can tell, this still exists and now refers to https://github.com/ros2/rosbag2/blob/master/rosbag2_cpp/src/rosbag2_cpp/readers/sequential_reader.cpp#L159

It seems to me that this simple check for nullptr is not of interest performance-wise, there are so many other things going on, that I am inclined to close this issue. @Karsten1987 if you think it's a problem, please reopen.

james-rms pushed a commit to james-rms/rosbag2 that referenced this issue Nov 17, 2022
* Cleanup in `mcap_vendor` package

- Use `${PROJECT_NAME}` instead of confusing `mcap` for library name and
exporting target, to be consistent with all other packages in ROS2.

- Add export for path to the `mcap` includes and export for mcap_vendor
library. It was an issue in downstream packages that `mcap/mcap.hpp` was
not visible since we were not exporting path to it explicitly.

- Removed test section with linters for `mcap_vendor` package. Since
package itself doesn't contain any tests and own source code and we
shouldn't run linters on any third party code. Prevent CI failures.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Install `mcap` headers in `include/${PROJECT_NAME}/mcap` and export them

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Revert renaming `mcap` library to `mcap_vendor` and fixed include issue

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Add `LIBRARY DESTINATION lib` for install targets

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Attempt to fix Windows build error by removing `ament_export_libraries(mcap)`

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Nitpick. remove extra space before `HAS_LIBRARY_TARGET`

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
james-rms pushed a commit to james-rms/rosbag2 that referenced this issue Nov 17, 2022
* Cleanup in `mcap_vendor` package

- Use `${PROJECT_NAME}` instead of confusing `mcap` for library name and
exporting target, to be consistent with all other packages in ROS2.

- Add export for path to the `mcap` includes and export for mcap_vendor
library. It was an issue in downstream packages that `mcap/mcap.hpp` was
not visible since we were not exporting path to it explicitly.

- Removed test section with linters for `mcap_vendor` package. Since
package itself doesn't contain any tests and own source code and we
shouldn't run linters on any third party code. Prevent CI failures.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Install `mcap` headers in `include/${PROJECT_NAME}/mcap` and export them

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Revert renaming `mcap` library to `mcap_vendor` and fixed include issue

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Add `LIBRARY DESTINATION lib` for install targets

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Attempt to fix Windows build error by removing `ament_export_libraries(mcap)`

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Nitpick. remove extra space before `HAS_LIBRARY_TARGET`

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: James Smith <james@foxglove.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed more-information-needed Further information is required
Projects
None yet
Development

No branches or pull requests

3 participants