search

facontidavide / ros_type_introspection

Deserialize ROS messages that are unknown at compilation time
MIT License
61 stars 30 forks source link

ANNOUNCEMENT: 1.0 beta #18

Closed facontidavide closed 7 years ago

facontidavide commented 7 years ago

https://github.com/facontidavide/ros_type_introspection/tree/dev

@ibtaylor @mehditlili , you are the users that contributed in the past, you might want to take a look before I merge into master.

I rewrote the library using a different interface. Originally I used a stateless API with 3 C like function.

The new approach use a class named Parser because in this way it is easier to keep a cache of some computed values as private members of the class. https://github.com/facontidavide/ros_type_introspection/blob/dev/include/ros_type_introspection/ros_introspection.hpp

The new implementation outperform the old one by a factor 2X.

Still the most expensive step is applyNameTransform, that takes much more than deserializeIntoFlatContainer.

The new implementation was tested successfully against all the rosbags I commonly parse for testing (and the unit tests, of course).

facontidavide commented 7 years ago

UPDATE: Shaving even more performance out of the benchmark.

Version 0.8 (master): 560 ms Version 1.0 beta (dev): 220 ms

ibtaylor commented 7 years ago

I dropped some comments on commits here (should see with little message box) https://github.com/facontidavide/ros_type_introspection/commits/dev

ibtaylor commented 7 years ago

fyi, in dev, tests fail

ibtaylor commented 7 years ago

these methods should probably be const methods as in void method() => void method() const since they do not/should not mutate object state https://github.com/facontidavide/ros_type_introspection/blob/418cc406dad847dfd4b08de629161dd284924361/include/ros_type_introspection/ros_introspection.hpp#L101 https://github.com/facontidavide/ros_type_introspection/blob/418cc406dad847dfd4b08de629161dd284924361/include/ros_type_introspection/ros_introspection.hpp#L110 https://github.com/facontidavide/ros_type_introspection/blob/418cc406dad847dfd4b08de629161dd284924361/include/ros_type_introspection/ros_introspection.hpp#L132-L135 https://github.com/facontidavide/ros_type_introspection/blob/418cc406dad847dfd4b08de629161dd284924361/include/ros_type_introspection/ros_introspection.hpp#L175-L177 this can be made const https://github.com/facontidavide/ros_type_introspection/blob/418cc406dad847dfd4b08de629161dd284924361/include/ros_type_introspection/ros_introspection.hpp#L157-L159 if https://github.com/facontidavide/ros_type_introspection/blob/418cc406dad847dfd4b08de629161dd284924361/src/ros_introspection.cpp#L469 is changed from operator[] which will do an insertion (unwanted probably) with .at()

ibtaylor commented 7 years ago

helper_funtions.hpp => helper_functions.hpp

facontidavide commented 7 years ago

thank you for the valuable remarks, I completely missed the typos and the const.

Test is not failing on my most up to date version. If it still fails on your PC, send me the error message.

ibtaylor commented 7 years ago

ran tests on commit c77c728356dea4bb2d2c1f6170f7c47b3ab2b741

[==========] Running 16 tests from 5 test cases.
[----------] Global test environment set-up.
[----------] 5 tests from RosType
[ RUN      ] RosType.builtin_int32
[       OK ] RosType.builtin_int32 (0 ms)
[ RUN      ] RosType.builtin_string
[       OK ] RosType.builtin_string (0 ms)
[ RUN      ] RosType.builtin_fixedlen_array
/data2/ws_gcc/ws_ros_lunar/src/ros_type_introspection/src/tests/parser_test.cpp:51: Failure
      Expected: f.arraySize()
      Which is: 0
To be equal to: 32
[  FAILED  ] RosType.builtin_fixedlen_array (0 ms)
[ RUN      ] RosType.builtin_dynamic_array
[       OK ] RosType.builtin_dynamic_array (0 ms)
[ RUN      ] RosType.no_builtin_array
[       OK ] RosType.no_builtin_array (0 ms)
[----------] 5 tests from RosType (1 ms total)

[----------] 4 tests from ROSMessageFields
[ RUN      ] ROSMessageFields.ParseComments
[       OK ] ROSMessageFields.ParseComments (0 ms)
[ RUN      ] ROSMessageFields.constant_uint8
[       OK ] ROSMessageFields.constant_uint8 (0 ms)
[ RUN      ] ROSMessageFields.ConstantNavstatus
[       OK ] ROSMessageFields.ConstantNavstatus (1 ms)
[ RUN      ] ROSMessageFields.ConstantComments
[       OK ] ROSMessageFields.ConstantComments (1 ms)
[----------] 4 tests from ROSMessageFields (2 ms total)

[----------] 2 tests from BuildROSTypeMapFromDefinition
[ RUN      ] BuildROSTypeMapFromDefinition.IMUparsing
/data2/ws_gcc/ws_ros_lunar/src/ros_type_introspection/src/tests/parser_test.cpp:255: Failure
      Expected: msg->field(2).arraySize()
      Which is: 5
To be equal to: 9
/data2/ws_gcc/ws_ros_lunar/src/ros_type_introspection/src/tests/parser_test.cpp:262: Failure
      Expected: msg->field(4).arraySize()
      Which is: 5
To be equal to: 9
/data2/ws_gcc/ws_ros_lunar/src/ros_type_introspection/src/tests/parser_test.cpp:269: Failure
      Expected: msg->field(6).arraySize()
      Which is: 0
To be equal to: 9
[  FAILED  ] BuildROSTypeMapFromDefinition.IMUparsing (3 ms)
[ RUN      ] BuildROSTypeMapFromDefinition.Int16MultiArrayParsing
[       OK ] BuildROSTypeMapFromDefinition.Int16MultiArrayParsing (1 ms)
[----------] 2 tests from BuildROSTypeMapFromDefinition (4 ms total)

[----------] 4 tests from Deserialize
[ RUN      ] Deserialize.JointState
[       OK ] Deserialize.JointState (2 ms)
[ RUN      ] Deserialize.NavSatStatus
[       OK ] Deserialize.NavSatStatus (1 ms)
[ RUN      ] Deserialize.DeserializeIMU
unknown file: Failure
C++ exception with description "buildRosFlatType: There was an error parsing the buffer" thrown in the test body.
[  FAILED  ] Deserialize.DeserializeIMU (2 ms)
[ RUN      ] Deserialize.Int16MultiArrayDeserialize
[       OK ] Deserialize.Int16MultiArrayDeserialize (1 ms)
[----------] 4 tests from Deserialize (6 ms total)

[----------] 1 test from Renamer2
[ RUN      ] Renamer2.DeserializeJointStateAndRename
[       OK ] Renamer2.DeserializeJointStateAndRename (2 ms)
[----------] 1 test from Renamer2 (2 ms total)

[----------] Global test environment tear-down
[==========] 16 tests from 5 test cases ran. (15 ms total)
[  PASSED  ] 13 tests.
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] RosType.builtin_fixedlen_array
[  FAILED  ] BuildROSTypeMapFromDefinition.IMUparsing
[  FAILED  ] Deserialize.DeserializeIMU

 3 FAILED TESTS
facontidavide commented 7 years ago

I do not have this problem. Which compiler are you using?

Which Boost version?

ibtaylor commented 7 years ago

I'm using ros lunar. Compiler gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4)

facontidavide commented 7 years ago

:( I will try to install a version of lunar and see why this is happening. It is super weird. Could it be a problem with atoi? Can you debug it on your computer and check the value of these lines for the failing tests? 

  std::string size(what[3].first, what[3].second);
  _array_size = size.empty() ? -1 : atoi(size.c_str());
ibtaylor commented 7 years ago

okay, I found that problem. assigning to type after the match will corrupt the results. doing

std::string type_tmp = type;
if (regex_search(type_tmp, what, array_regex))

works fine

facontidavide commented 7 years ago

Awesome! Just pushed the fix

facontidavide commented 7 years ago

I am about to close this and merge dev into master (and release it on ROS Indigo, Kinetic and Lunar).

The refactoring is not 3X faster than the original code and looking at the results of Callgrind I can not see any low hanging fruit for further optimization.

facontidavide commented 7 years ago

pushed