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

Performance benchmarking refactor #594

Merged
merged 29 commits into from
Jan 26, 2021

Conversation

adamdbrw
Copy link
Collaborator

@adamdbrw adamdbrw commented Jan 4, 2021

Improvements to current performance benchmarking package:

  • extended to include benchmarks of end-to-end performance of recording (including transport).
  • reuse of byte producer for both transport-less writer and end-to-end
  • YAML configuration enabling mixed publishers, defined as groups (to simplify adding hundreds or thousands of similar publishers).
  • configuration of QoS settings of publishers
  • use new configuration in writer_benchmark as well
  • replace non-portable and clunky benchmark script with launch / python based experiment batch execution
  • add description on how to run it to readme
  • count/stats for end-to-end benchmarks
  • add end-to-end experiments to launch
  • run (more) experiments with new setups. Compare trasport vs no-transport.
  • only record interesting topics so that stats are not disturbed (i.e. no logs or parameters) - this was solved with Recorder --regex and --exclude options #604.

@adamdbrw adamdbrw requested a review from a team as a code owner January 4, 2021 16:15
@adamdbrw adamdbrw requested review from Karsten1987, jaisontj and mjeronimo and removed request for a team January 4, 2021 16:15
@adamdbrw adamdbrw marked this pull request as draft January 4, 2021 16:16
@adamdbrw adamdbrw force-pushed the performance_benchmarking_refactor branch from e0e6118 to c0bcbf1 Compare January 11, 2021 14:49
@pijaro pijaro force-pushed the performance_benchmarking_refactor branch from 1294602 to 5a8e6f2 Compare January 12, 2021 14:29
@adamdbrw adamdbrw marked this pull request as ready for review January 20, 2021 10:57
@adamdbrw
Copy link
Collaborator Author

@mjeronimo @emersonknapp @Karsten1987 this is ready for a review now

@adamdbrw adamdbrw changed the title [WIP] Performance benchmarking refactor Performance benchmarking refactor Jan 20, 2021
@adamdbrw adamdbrw force-pushed the performance_benchmarking_refactor branch 3 times, most recently from 7219b37 to c6ef8ff Compare January 22, 2021 14:25
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Pretty complicated - very big review, but since it's not direct product code, i'm inclined to merge it if it works as desired

@adamdbrw
Copy link
Collaborator Author

We already used it to run numerous experiments (reports are incoming) and it works for us reliably. I agree it can be difficult to review since it is quite big.

@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/7bc8c2396115fc7b1d52a2fc8f8c9ea0/raw/7d026faddaa306b579571fb44d4d3ae65f33f992/ros2.repos
BUILD args: --packages-up-to rosbag2_performance
TEST args: --packages-select rosbag2_performance
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7510

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

@adamdbrw
Copy link
Collaborator Author

adamdbrw commented Jan 25, 2021

test failure: since the code of this PR is contained in an independent package that in no way affects rosbag2 recording, I assume that this is a flaky test.
CI job failures: I am not entirely sure, but could it be due to the fact that the benchmarking package requires --cmake-flags -DBUILD_ROSBAG2_BENCHMARKS in colcon build arguments? This however didn't change with this PR, since the previous version of the package required the same flag.

Edit: corrected spelling of the flag.

@emersonknapp
Copy link
Collaborator

emersonknapp commented Jan 25, 2021

CI job failures: I am not entirely sure, but could it be due to the fact that the benchmarking package requires --cmake-flags -DBUILD_ROSBAG2_BENCHMARKING in colcon build arguments

I have added that, but the main problem is my mistake: I asked for package rosbag2_performance when it is called rosbag2_performance_benchmarking

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

@adamdbrw
Copy link
Collaborator Author

adamdbrw commented Jan 25, 2021

The flag needs to be --cmake-args -DBUILD_ROSBAG2_BENCHMARKS=1. My apologies for giving a wrong spelling and no value in an earlier comment (this is in the README, but I guess the late hour works against me)

@adamdbrw
Copy link
Collaborator Author

adamdbrw commented Jan 26, 2021

I fixed Windows build issue with uint (which is an usual compiler shortcut but not supported by all compilers). Perhaps all will be green this time :]
It seems that there are build issues in compression code (not affected by this PR)

- renamed since now it no longer benchmarks writer only
- generalized byte_producer so that it uses a callback instead of queue, so it can be reused in publisher scheme

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
- can specify multiple groups of publishers (see attached example yaml)
- reuses byte producer

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Also included in yaml example.

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
adamdbrw and others added 21 commits January 26, 2021 12:39
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

added comment in storage_optimized.yaml
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
…ild)

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
@adamdbrw adamdbrw force-pushed the performance_benchmarking_refactor branch from 2f1066c to fe8b6b2 Compare January 26, 2021 11:40
@emersonknapp
Copy link
Collaborator

It seems that there are build issues in compression code (not affected by this PR)

Where? If you mean the "Rpr" build - it isn't a good indicator of success, because it only builds against released versions of packages, rather than against the latest version of the code. In this case, rcpputils has a new const-specifier that is merged in master but not released in rolling yet - I would ignore that build and pay attention to "Test rosbag2 / build_and_test (pull_request) " and the ci.ros2.org builds in general, which both build from source

@emersonknapp
Copy link
Collaborator

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

@emersonknapp emersonknapp merged commit 6e455c1 into master Jan 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the performance_benchmarking_refactor branch January 26, 2021 20:18
@y-okumura-isp
Copy link

Are there any plans to backport it to Foxy?

@adamdbrw
Copy link
Collaborator Author

adamdbrw commented Feb 2, 2021

@y-okumura-isp I will soon be able to update you on this, since it is a ongoing decision process that also needs feedback from the community. If I can support you in the meanwhile, please let me know - you can find my email in the commit log.

The same applies to double buffering PR (#546) - I saw you were also interested in this one.

emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
* Refactoring of rosbag2 performance package:
- renamed since now it no longer benchmarks writer only
- generalized byte_producer so that it uses a callback instead of queue, so it can be reused in publisher scheme

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Benchmark publishers based on yaml configuration
- can specify multiple groups of publishers (see attached example yaml)
- reuses byte producer

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Applying configured QoS settings for publishers.
Also included in yaml example.

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* linters

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Towards common configuration
- separating out common structures
- utility class for common parameter parsing

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Barebone launchfile for benchmarks.

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* writer benchmark adapted to yaml file and publisher groups

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* refactored result writing and bag parameters

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* linters

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Launchfile for benchmarks

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Change storage config file from non optimized to resilient

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Max bag size for benchmark launchfile. Launchfile refactor.

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Copy yaml configs after benchmark is finished.

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Benchmark results csv file extended

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* added disclaimer about random data and compression

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Report gen tool for benchmarks

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Benchmarks out dir name changed

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* results writer node

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* documentation

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Transport and transportless in launchfile

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Benchmark launchfile refactor

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Wait for rosbag listening in benchmark launchfile

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Uncrustify and some comments for benchmarking tools

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Added new producers config for benchmarks

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Missing parameters in transport benchmark

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

added comment in storage_optimized.yaml

* Missing rosbag record parameters in transport benchmark

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Wait for subscriptions parameter in producers config

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* moved utils code from header to source

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* changed compiler shortcut uint to unsigned int (should fix Windows build)

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

Co-authored-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
* Refactoring of rosbag2 performance package:
- renamed since now it no longer benchmarks writer only
- generalized byte_producer so that it uses a callback instead of queue, so it can be reused in publisher scheme

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Benchmark publishers based on yaml configuration
- can specify multiple groups of publishers (see attached example yaml)
- reuses byte producer

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Applying configured QoS settings for publishers.
Also included in yaml example.

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* linters

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Towards common configuration
- separating out common structures
- utility class for common parameter parsing

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Barebone launchfile for benchmarks.

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* writer benchmark adapted to yaml file and publisher groups

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* refactored result writing and bag parameters

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* linters

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Launchfile for benchmarks

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Change storage config file from non optimized to resilient

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Max bag size for benchmark launchfile. Launchfile refactor.

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Copy yaml configs after benchmark is finished.

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Benchmark results csv file extended

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* added disclaimer about random data and compression

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Report gen tool for benchmarks

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Benchmarks out dir name changed

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* results writer node

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* documentation

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* Transport and transportless in launchfile

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Benchmark launchfile refactor

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Wait for rosbag listening in benchmark launchfile

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Uncrustify and some comments for benchmarking tools

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Added new producers config for benchmarks

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Missing parameters in transport benchmark

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

added comment in storage_optimized.yaml

* Missing rosbag record parameters in transport benchmark

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* Wait for subscriptions parameter in producers config

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

* moved utils code from header to source

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* changed compiler shortcut uint to unsigned int (should fix Windows build)

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

Co-authored-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants