search

ApexAI / performance_test

**This project is deprecated** Go to https://gitlab.com/ApexAI/performance_test
64 stars 41 forks source link

ROS2 Polling Subscription Plugin #88

Closed daggarwa closed 5 years ago

daggarwa commented 5 years ago

Added polling subscription plugin for ROS 2 to the performance test. It can be used by specifying the argument -c [ --communication ] arg Communication plugin to use as ROS2PollingSub when running performance test .

To turn on the option for using Polling Subscription Plugin we need to compile the performance test with cmake args PERFORMANCE_TEST_POLLING_SUBSCRIPTION_ENABLED and value as ON :

colcon build --cmake-clean-cache --cmake-args -DPERFORMANCE_TEST_POLLING_SUBSCRIPTION_ENABLED=ON

Sample usage for the plugin can be :

ros2 run performance_test perf_test -c ROS2PollingSubscription -l log -t Array16k --history_depth 100 --max_runtime 60 -p 1 -s 1 -r 20 --reliable --keep_last`

I have verified that this plugin builds and runs successfully on ApexOS version : 0d34c574917d

This change is Reviewable

monidzik commented 5 years ago

@daggarwa you should put some information about this new option in readme file.

daggarwa commented 5 years ago

@daggarwa you should put some information about this new option in readme file.

@monidzik Sounds good! I have added the cmake-args info for ROS2PollingSub here like Cyclone DDS and others : https://github.com/ApexAI/performance_test/commit/fa25cd1a7aa72acf8e8928737799a9fcda81b04a#diff-fcc41d9bd699e1a53818d508382f0e0aR37

daggarwa commented 5 years ago

I know you are working on correcting the qos settings so I can compile your branch, so let me check your code again when it's ready to run and CI is passing.

@monidzik I have made the requested changes (most of them except one : https://github.com/ApexAI/performance_test/pull/88#discussion_r328612243) need your input on that. Also the CI passes now. I believe the PR is ready for a second round of review. I have assigned it to you again. Thanks!

daggarwa commented 5 years ago

@monidzik I guess I have addressed all your review comments. Thanks so much for the review. As discussed I am now assigning the PR to @deeplearningrobotics for further review.

daggarwa commented 5 years ago

Ok, you still need to add the readme information with the option description, otherwise I have only couple of small remarks.

@monidzik I forgot to respond on this earlier. I can do the update to the readme now and push that.

deeplearningrobotics commented 5 years ago

@monidzik: Please also review my comments.

daggarwa commented 5 years ago

@monidzik: Please also review my comments.

@deeplearningrobotics @monidzik I have addressed's all of @deeplearningrobotics 's comments. Please review once. Thanks.

monidzik commented 5 years ago

@monidzik: Please also review my comments.

@deeplearningrobotics @monidzik I have addressed's all of @deeplearningrobotics 's comments. Please review once. Thanks.

@daggarwa ok, next time please just assign to me, I didn't see this message, sorry!

monidzik commented 5 years ago

@deeplearningrobotics Divya has some comments for you to resolve, so leaving them for you!

daggarwa commented 5 years ago

@monidzik: Please also review my comments.

@deeplearningrobotics @monidzik I have addressed's all of @deeplearningrobotics 's comments. Please review once. Thanks.

@daggarwa ok, next time please just assign to me, I didn't see this message, sorry!

@monidzik Okay I will do that from next time. Thanks for reviewing this.

daggarwa commented 5 years ago

@deeplearningrobotics As discussed in the morning, I have committed changes to resolve https://github.com/ApexAI/performance_test/pull/88#discussion_r329707857 and https://github.com/ApexAI/performance_test/pull/88#discussion_r329718116

Can you please review the changes here : https://github.com/ApexAI/performance_test/pull/88/commits/aa11e1d46ca92f5ec3fd146a752e459cc7290e41 and continue your review of the PR. Thanks

daggarwa commented 5 years ago

@deeplearningrobotics I think I have addressed all your comments now. Can you please review once. Assigning back to you.

deeplearningrobotics commented 5 years ago

@monidzik: I reviewed the changes and the code looks good to me. Please test all the communication plugins and check if everything is still functional and then approve.

@daggarwa: FYI

deeplearningrobotics commented 5 years ago

@daggarwa: Can you also run it trough pref to make sure we did not introduce performance bottlenecks.

dejanpan commented 5 years ago

@daggarwa: Can you also run it trough pref to make sure we did not introduce performance bottlenecks.

@deeplearningrobotics is "pref" a typo? Did you mean https://perf.wiki.kernel.org/index.php/Tutorial?

It is possible that @daggarwa will not understand what is meant by running it through perf - can you point her to what explicitly should be done?

deeplearningrobotics commented 5 years ago

@daggarwa: Can you also run it trough pref to make sure we did not introduce performance bottlenecks.

@deeplearningrobotics is "pref" a typo? Did you mean https://perf.wiki.kernel.org/index.php/Tutorial?

It is possible that @daggarwa will not understand what is meant by running it through perf - can you point her to what explicitly should be done?

@dejanpan: Yes, I mean perf sorry.

@daggarwa: Lets look together at the perf graphs when you are back at the office so I can show you what to look out for.

daggarwa commented 5 years ago

@daggarwa: Can you also run it trough pref to make sure we did not introduce performance bottlenecks.

@deeplearningrobotics is "pref" a typo? Did you mean https://perf.wiki.kernel.org/index.php/Tutorial? It is possible that @daggarwa will not understand what is meant by running it through perf - can you point her to what explicitly should be done?

@dejanpan: Yes, I mean perf sorry.

@daggarwa: Lets look together at the perf graphs when you are back at the office so I can show you what to look out for.

@dejanpan Thank you for the clarifying question. @deeplearningrobotics Okay sounds good.

daggarwa commented 5 years ago

@daggarwa looks good to me, I checked also other communication means and everything works fine. The only thing is that you are still not mentioning anything about this option in readme - did you decide not to write there anything?

@monidzik I am planning to add ROS2 Waitset like here : https://github.com/ApexAI/performance_test/pull/88/commits/09fd145426e6cb3ca501d7ac9a4b0d84d30121e5 to be consistent with other implemented plugins

daggarwa commented 5 years ago

@daggarwa looks good to me, I checked also other communication means and everything works fine. The only thing is that you are still not mentioning anything about this option in readme - did you decide not to write there anything?

@monidzik I am planning to add ROS2 Waitset like here : 09fd145 to be consistent with other implemented plugins

@monidzik I have added the changes above to the readme. Kindly review.

daggarwa commented 5 years ago

@deeplearningrobotics The profiling with perf tool is done. There are no performance impacts if we add this plugin in. Also , rebased the branch off master and cleaned up the commits. Please let me know if anything else is required before it gets merged in! Thanks