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

Added benchmark test to rmw_implementation #127

Merged
merged 7 commits into from
Sep 29, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Sep 16, 2020

Most of the content here builds on rcpputils' shared library code and doesn't add any complexity worth testing. However, the preload operation lists quite a lot of symbols, and we should track the performance of that operation.

I tested rmw_init() which is in charge of fetch the initial symbols

Signed-off-by: ahcorde ahcorde@gmail.com

@ahcorde ahcorde added the tests Failing or missing tests label Sep 16, 2020
@ahcorde ahcorde self-assigned this Sep 16, 2020
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/benchmark/rmw_implementation branch from cd49491 to e0dcadf Compare September 16, 2020 12:38
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 16, 2020

Compiling up-to test_rmw_implementation and testing test_rmw_implementation

With -DAMENT_RUN_PERFORMANCE_TESTS=ON:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good - I like that it runs for each of the RMWs. Would you mind renaming it to rmw_init since it's testing a lot more than just the prefetch function?

There is one big caveat here: library loading is only happening for the first call to rmw_init. For example, if one of the RMW libraries has a massive amount of shared libraries linked to it, we'll only pay the price of loading them all once, and then each iteration after that won't need to do it again. After looking at the implementation, I don't think we want to expose the additional API on rmw_implementation that would allow us to explicitly unload the library during each timing iteration, so these results will always be a best-case scenario.

Working off from this PR and also #135, I was able to confirm that forcing the library load causes the rmw_init duration to increase by around 20%, so I think that's something we'll want to track.

I'm really not sure why rmw_implementation has a separate package for tests, but if it's possible to move these tests into the rmw_implementation package itself, we can add some purpose-built functions to care of unloading.

test_rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 25, 2020

@cottsay, moved benchmark test to rmw_implementation

Signed-off-by: Scott K Logan <logans@cottsay.net>
This can be used to test the loading sequence for each RMW. I can
already see that there is some investigation to do for Cyclone DDS.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay merged commit c9c13dc into master Sep 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the ahcorde/benchmark/rmw_implementation branch September 29, 2020 00:41
ahcorde added a commit that referenced this pull request Oct 21, 2020
Co-authored-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: ahcorde <ahcorde@gmail.com>
ahcorde added a commit that referenced this pull request Oct 23, 2020
Co-authored-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Failing or missing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants