-
Notifications
You must be signed in to change notification settings - Fork 119
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
Static typesupport #203
Static typesupport #203
Conversation
Can you please reference the exact messages which are being used in the "Performance test results" table. I expected a "big" improvement for nested messages with many fields. I am a little bit surprised about the huge difference for a 8 MB point cloud. Do you have any more information why that is the case? |
I edited the initial message including links to the message definitions on the tables. I also changed the allocation results table to only depict improvements. In general, the main improvement is that we get rid of a temporary FastBuffer object. That means we are always saving one allocation of the default buffer size, and one copy of the whole serialized message. On serialization, for some messages (including PointCloud8m) we are saving reallocations of that temporary buffer. I think that in the case of big messages (like PointCloud8m) the main improvement comes from the saving of the copy from the temporary buffer to the definitive one. |
Could these changes also be applied to the dynamic code path? Or are the only feasible in the static case? Could you reference the exact code line which you refer to in order to ease digging into this. |
I think we could apply most of the changes to the dynamic code path. I will look into it next week while I perform the refactor to have dynamic and static support on different packages
Sure. For the publishing part, the related code is here rmw_fastrtps/rmw_fastrtps_cpp/src/rmw_publish.cpp Lines 43 to 50 in 3700d75
Allocation and reallocations are performed in calls to the serialization methods of Cdr inside _serialize_ros_message . The final copy is done inside info->publisher_->write , exactly here:
For deserialization, an initial allocation and copy is performed here: rmw_fastrtps/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.hpp Lines 792 to 794 in 3700d75
called from: rmw_fastrtps/rmw_fastrtps_cpp/src/rmw_take.cpp Lines 47 to 50 in 3700d75
|
Just adding a comment to let you know that refactor is complete. @dirk-thomas I have updated my todo list to include the changes to remove the temporary buffer from the dynamic code path. If you prefer that to be on another PR, just let me know. |
Yes, please create a separate PR for that so that they can be considered/ reviewed / merged independently. |
The current patch has conflicts and is not mergable. Please address those. For the review as well as readability of the history can you please have separate commits for moving / copying existing files. And apply potential changes after that in separate commits. That will allow us to review only actual changes. |
2aa0cbb
to
d542c14
Compare
Ok @dirk-thomas. I have refactored the commit history so the PR is ready for review. |
explicit ServiceListener(CustomServiceInfo * info) | ||
: info_(info), list_has_data_(false), | ||
conditionMutex_(nullptr), conditionVariable_(nullptr) | ||
inline bool take(void * ros_request, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if at least all classes and public functions could be documented.
@MiguelCompany: I see you are creating a |
The first commit is not a 1-to-1 copy of the existing files. Therefore git can't follow any history for these files. Please split that commit into two: the first doing a 1-to-1 copy of the existing files (without any changes), the second applying the necessary changes (renaming the package, namespace, etc.). Same for the later commit copying the file into I also noticed that a later commit also deletes some of the files previously added (e.g. `rmw_fastrtps_shared_cpp/src/type_support_common.hpp). Please don't copy those in the first place to reduce the amount of changes. Also #192 has been merged recently and you patch seems to revert that change. Nitpick: the copyright year you usually "extend" rather than replace: |
To @andreaspasternak
This is what happens in the code currently on master. The main improvement on the static typesupport is that we get rid of that temporary FastBuffer creation.
In the code currently on master, yes. Has also been removed on static typesupport |
fba7e0a
to
3cda3ad
Compare
To @dirk-thomas
Done
You're right. Done. Thanks.
You're right! It was partially reverted! It's fixed now |
@MiguelCompany: I tried to test this code by cloning it into my ROS2 workspace. But I get the following error:
Can you tell me how I can test it? |
@andreaspasternak It looks like you have an earlier version of ament_cmake. This option has been added after the ardent release: ament/ament_cmake#123 |
@mikaelarguedas: Great, I will try to install the newest ROS 2 version. |
@mikaelarguedas, @MiguelCompany: So I built ROS 2 from sources, using the This built fine. Then I switched to this branch (
I guess the issue is this line here:
So what else do I need to do the get it built? Do I need a different FastRTPS version? PS: I also had to make this small change
|
3cda3ad
to
b09afa8
Compare
Hi @andreaspasternak Seems I forgot to add some changes when rearranging my commits. It should be fixed now |
@MiguelCompany: Great, it compiles now. Thank you! |
7638276
to
4515414
Compare
4515414
to
1429803
Compare
Hey @dirk-thomas, I just rebased on top of master. Could this be reviewed and merged? |
@dirk-thomas: Can we support you somehow to get this merged? |
We are currently waiting for an internal discussion about the desired names of the rmw implementation packages ( -- I think the current conflict is due to #214. Instead of rebasing this huge patch I would suggest we revert the style patch, then this PR can be applied as is and then reapply the style fix. So nothing need to be done in this PR for now. |
Fine by me if reverting #214 resolves the conflict we can reapply it after this being merged. Though we may want to run uncrustify on this patch as well and fix the style violations before merging. |
I agree with this. I have just added a commit with changes equivalent to the ones in #214. |
@MiguelCompany: While testing this PR: ApexAI/performance_test#2 we run into problems building |
@deeplearningrobotics No. This PR does not require |
@MiguelCompany: Can you elaborate this process? Could you also explain the future then of |
@deeplearningrobotics Sure. Package rosidl_parser exports the classes needed to represent the structure of idl files. It also exports functions This means that, as the parsing of the idl files is already been done by ROS 2 code, if the switch from msg to idl format is performed, I assume the rosidl_parser package will be updated accordingly. We will still support fastrtpsgen for applications outside ROS 2, of course. |
@ros2/team are we going to resolve the merge conflict by reverting the style commit or is this a new merge conflict? It would be nice to close this pr out to avoid it continually getting new conflicts. |
Yes, as mentioned in my comment above: #203 (comment)
It is still waiting for a decision on the package naming. |
From #203 (comment) I think the only thing pending is the decision on the naming and the default implementation |
Sorry, I knew you guys had a plan, but I thought (incorrectly) that @MiguelCompany might of fixed it in 9b3be5d and since then a new conflict was introduced, but it looks like that's not the case. |
@dirk-thomas, @wjwwood, @MiguelCompany: How can we get that merged? Please tell me if you need any support It is pending for 2 months now. |
In today's meeting we decided to merge the patch as is ( I will work on merging this and correlated patches to go along with the newly introduced rmw package name tomorrow... |
Please see #218 which has been merged and contains the patches from this PR. |
Summary
At eProsima we are working on static typesupport for rmw_fastrtps. We now have the basic functionality ready for C++, but we still have the following todos:
Performance test
We have run some performance tests (which code is also in the repo). The test scenario involves two applications:
Listener application with subscriber for "test" and publisher for "test_echo":
Talker application with publisher for "test" and subscriber for "test_echo":
For each data type we run the test as follows:
Performance test results
Depending on the data type, we see an improvement from 0.15% to 84%. The improvement increases with the size of the message as well as with its complexity (number of fields / nested submessages).
("Hello World")
(16KB static array)
(4MB static array)
(512KB static array + PointField [8])
(8MB static array + PointField [8])
(16 byte fields)
(8 Struct4k fields, each with 16 Struct256 fields, each with 16 Struct16 fields)
(seq. of 1024 strings + seq. of 1024 Transform + seq. of 1024 Twist + seq. of 1024 Wrench)
Allocation test
We have also run a different test scenario to compare memory allocation and copy. In this scenario (launching the applications with argument -m) there is no echo, just a standard talker and listener, and a fixed number of messages (30) is published once a second 30 times. This way, we know the serialization is done the same number of times.
We run the talker and the listener under valgrind tools 'massif' (to compare peak size of heap) and 'callgrind' (to compare number of calls to malloc, realloc, memcpy and such)
Allocation test results
The results show that peak size of heap is almost the same for both branches. The main improvements are in calls to allocation functions.
Results show that, on serialization, we are allways saving one call to malloc per message. We are also saving realloc calls depending on the size and complexity of the message (up to 860 per message!). For deserialization, one call to malloc is saved per message.