performance testing packages

[adamdbrw]

Package for performance #435 metaticket tasks.

Builds with the rosbag2 project, but you will need additional dependencies. Please see readme for instructions on how to use it.

This is the first package out of 2 (plus one supporting) benchmarking packages for rosbag2. This one focuses on replicating and benchmarking the part when the message already arrives to the recorder through the ROS2 transport (so, doesn't include transport layer performance).

Current functionality:

• Contains scalable and configurable producers of data
• The build is controlled by a dedicated flag (BUILD_BENCHMARKS)
• Counts received messages and percentage of all received.
• Includes a simple convenience script to run batches with different parametrization and produce output files (which can be easily grepped and processed).

What is not there yet (proposed enhancements):

• automatic processing of output files from the batch run to produce a summary. It could be easily realized through use of grep or through adding a joint, csv (or similar) format file as an output.
• integration with an automated build processes
• support for compression and conversion. Both are relatively easy to add.
• writer_benchmark doesn't need to be a ROS node. This could be removed with logging and parameters replace by non-ROS solutions. However, it uses rcl and rmw calls to mimic what is happening in the real recorder code (which is arguably worth enough not to make it ROS-agnostic).

[adamdbrw]

Example partial result for a case with 10Kb messages with 100 topics at 100Hz. Note that there are some issues still and the summary report doesn't generate properly yet.

bechmark:
  title: 10KB with 100 topics
cpu:
  mean: 17.611134601832273
  std: 14.737801039292737
disk:
  read:
    mean: 0.0003239325842696629
    std: 0.003038756996757516
  write:
    mean: 94.42021651685393
    std: 53.916070906797025
memory:
  max: 6849.572864
  mean: 6590.834894387596
  min: 6412.9024
  std: 132.1152373612786
messages:
  captured: 36629
  expected: 100000
  percent: 36.629

Example summary table (performed on a different hard drive):

For comparison, a table posted in this article

[adamdbrw]

@emersonknapp Just a kind reminder that this is ready for a review

[emersonknapp]

Thanks for the reminder, we'll start taking a look!

[Karsten1987]

I am sorry but this PR is really rough to review. It's not really clear what's happening and moreover how this is supposed to be used later on - ideally on a per PR basis.

I understand that we have the config files to create/mock different scenarios on how much payload is being transmitted. What I don't understand is all the cached console outputs. Furthermore, there seems to be quite some launch files present where a handful of ROS nodes are being spawned. Why do we need all this? Why is ROS involved in this in the first place? Given that we only want to benchmark the performance of the ROS agnostic part of rosbag2 (meaning rosbag2_storage and the sqlite3 default plugin) I don't see the need to introduce ROS nodes.
The transport part of this PR is not clear to me. What are dummy publishers and what does "warming up topics" mean? Secondly, there are some scripts which seem to tamper with the user's global system - copying files to /usr/share/.. as well as running scripts with administrative rights. I think that's a no-go here for this benchmarking purpose.

I would kindly ask you to break this PR apart into semantically independent pieces (if possible). The first thing I'd imagine is the actual ros-agnostic performance benchmark, which utilizes the queues to simulate a mock rosbag2-transport. I would imagine that this script generates some machine readable code (csv or yaml files) which can then later on be parsed and plotted.
I guess all other files can actually be part of a second follow up PR. But this way, it would allow us to get started and iterate quicker of this.

[Karsten1987]

Suggested change

# REPORT_DIR=~/.ros/reports

[pijaro]

To be fixed!

[Karsten1987]

Why is this file called voyager? Is that related to the medium article? If so, I honestly don't see much value in having this script here.
I would rather have the data prepared in a isolated rosbag2 centered way, meaning to have stats which can be compared against it other, not against external benchmarks.

[pijaro]

Definitely a good idea, this would be a way better approach.

[Karsten1987]

newline missing

[Karsten1987]

same in all other yaml files below.

[pijaro]

To be fixed!

[Karsten1987]

I guess this could be more specific, something like "rosbag2_benchmarking".

[pijaro]

You are right, will be changed!

[Karsten1987]

what is that? What is "warming up topics"?

[pijaro]

Warming up topics basically means to let rosbag2 know about every topic we are going to use before we start any worker or monitoring tool. The reasoning behind this is as follows: it makes the synchronization process much cleaner – we are not relying on different workers to pop up the discover on rosbag2, we just want them to produce data as soon as rosbag2 is ready; discovering large number of topics (thousands) at the same time usually leads to some topics not being discovered by rosbag2 – that's why “warming topics” tool is doing that in a more staggered fashion; we tried to be focused as much on utilizing the writing capabilities of rosbag2, not the discovery issues.

Now I see that this mechanic may be too strange and unnecessary after all. I'm thinking about an alternative checks for topics existence and probably will propose some different and lighter approach.

[Karsten1987]

why do I need this? What's the difference to ros2 topic pub <topic> <msg_type>?

[pijaro]

Launching ros2 topic pub looked pretty slow, what is more, launching 1000 instances was way slower 😃

[Karsten1987]

should that file be renamed to report?

[pijaro]

Yes, it should. Will fix it, thanks 😃

[Karsten1987]

why do we need a rclpy node for generating a report?

[pijaro]

You are correct, we should not need rclpy for that. It was used to keep the consistency in using running arguments (ie --ros-args -p foo:=bar). Will be fixed.

[Karsten1987]

so this wouldn't run on windows. Not that it's super important, but I feel we should have a sanity check here that the exec is actually available.

[pijaro]

Good point. I will add OS check here.

[Karsten1987]

👎 I don't appreciate that we modify users configs here.

[pijaro]

Yes, indeed we included an iotop custom policy here and modifying anything in usr/share is controversial at best. However, using this policy is not necessary. It is just a noticeable quality of live improvement for large test bundles. User can still enter password before each test and everything will be fine.

Sudo is required because of the system utilization tool we are using (iotop) and this is reasonable because we are literally recording system stats at the time.

We don’t really appreciate that as well so we will look for an different tool which will not require root privileges. Alternatively we can think about moving out the iotop instance from single benchmark to something that will run more globally and require only one sudo prompt. I will look into that!

[adamdbrw]

Thank you for the review! A general comment:
There are two separate packages for benchmarking: full stack, including transport - package rosbag2_benchmarking, and without transport - package writer_benchmarking. These roles are also described in the top-level README file. Both approaches are intended. These packages are quite different and separate from each other - and that would be my first idea to break the PR into pieces. Would you like us to break the PR this way?

@pijaro will handle the review remarks of rosbag2_benchmarking (his package and he is better at Python ;) ), while I handle writer_benchmarking part

[pijaro]

Thanks for review, we really appreciate that! We will put some fixes soon and then split this PR into pieces.

[Karsten1987]

These packages are quite different and separate from each other - and that would be my first idea to break the PR into pieces. Would you like us to break the PR this way?

Yes, this would make a lot of sense to me. First, a ROS agnostic benchmark package which tests the storage backends: Ideally, this should not only test sqlite3, but should be possible to test any other backend as well.

Why are these packages different though? I was wondering why browsing through the code, can the package testing transport not rely on the ROS agnostic package? That way, changing things in one more package also clearly affects the other one and doesn't have to be fixed twice.

Generally, it makes the review a bit easier if all files which are no longer needed are removed from this PR.

[Karsten1987]

@adamdbrw @pijaro do you have any update on this PR? I have a few ideas on how to address the findings in the performance reports, but they are somehow blocked by this PR. I think we have to have the benchmarking tool in place before reviewing further performance PRs in order to verify that these are actual improvements.

[adamdbrw]

@Karsten1987 - I have trimmed down this PR to the single package to streamline our process. The second benchmarking package (which includes transport) will be added separately after this PR review is over.

I have updated this PR description - there is also a list of proposed enhancements, which are known shortcomings of the performance package. Please let me know what is required for this package to be useful to you right away, and what you think can be raised as an enhancement issue. I will try to do my best for this to be merged!

As for this question:

Why are these packages different though? I was wondering why browsing through the code, can the package testing transport not rely on the ROS agnostic package? That way, changing things in one more package also clearly affects the other one and doesn't have to be fixed twice.

We can wait for the second PR to handle this. In my opinion they are good as independent of each other since the other one does black-box testing - and the common code is already separated out - it is the rosbag2 code.

[Karsten1987]

Thanks a bunch @adamdbrw for coming up with this reformatting. It's way easier to review now.

I just did a first pass, a few things I think could be considered. One thing I would love to see though is that instead of "just" printing the benchmark results, the values are stored in sort of a csv or txt file. This would allow us to write follow up script parsing these files for plots etc.

As for style, I know it's not the most important thing here for this package, but consider adhering to the ROS2 style guide: https://index.ros.org/doc/ros2/Contributing/Code-Style-Language-Versions/#id1
Functions are snake_case and private member functions are suffixed with an underscore instead of mVar. You might also activate the linters to see that cppcheck and stuff passes.

[adamdbrw]

Thank you for the review! I applied most of suggestion and asked questions about the rest.

As for style, I know it's not the most important thing here for this package, but consider adhering to the ROS2 style guide: https://index.ros.org/doc/ros2/Contributing/Code-Style-Language-Versions/#id1
Functions are snake_case and private member functions are suffixed with an underscore instead of mVar. You might also activate the linters to see that cppcheck and stuff passes.

I ran both ament_cppcheck and ament_cpplint (previously and now again)
The only complain is about the header guards (i.e. BYTE_PRODUCER_HPP_ is preferred by the linter) - which I ignore according to your suggestion. It doesn't complain about mVar and camelCase for function names. Would you still prefer me to change them to _var and snake_case (no problem)?

As for storing the values in files, the current script (benchmark.sh) outputs a bunch of .log files, which can be processed automatically, although with some grepping. My idea is to keep these logs (they include a nice overview of how the test ran), but also add a summary csv file to which each benchmark will contribute and which would contain one line per setup, constructed in such a way:

... <messages_produced> <messages_lost>

Let me know if you would like something else.

[Karsten1987]

It doesn't complain about mVar and camelCase for function names. Would you still prefer me to change them to _var and snake_case (no problem)?

It won't complain in this case. It' solely the convention within the ROS2 code base. So it's preferred, but not enforced. Your call ;-)

As for storing the values in files, the current script (benchmark.sh) outputs a bunch of .log files, which can be processed automatically, although with some grepping.

I've just realized this. I would be very much in favor if we could change this behavior to output the log files directly within the benchmarking code. Writing these data to file within C++ should really be straight forward. Grepping the stderr seems unnecessarily complicated for this and error prone if case other prints are interleaved at some point.

[adamdbrw]

Alright, time for a second round! We are getting to it 👍
Could you look also at my responses to your earlier suggestions? Not everything was merged and perhaps some still should.

[adamdbrw]

@Karsten1987 take a look at the latest changes:

• applied the remaining two review comments
• renamed member variables and functions to _variable and _snake_case
• implemented logging of results to common csv file which is straightforward to plot

With the last change, the results of a benchmark now also reside in a subfolder and there is a summary csv file there. I kept the log files as they can serve to diagnose what happened during execution of each benchmark.

Attached: an example file from a test run of 10Mb/s benchmarks (changed to txt extension because github complains about csv)
results.txt

[adamdbrw]

The tree should look good now, though through my error this PR history is difficult to read

[Karsten1987]

thanks for rebasing. We have to iterate once more over the bash script. The output directory is being created by rosbag2_cpp itself on master, where the script creates these. The script will now crash.

Second, I would appreciate if we could remap the node name (currently hardcoded to benchmark_writer) to reflect the current running instance to avoid any node conflicts.

[adamdbrw]

Done - please take a look if this is ok now

[Karsten1987]

this looks good to me so far. There are a couple of things (e.g. writing into different topics other than one, rule out race conditions such as #486) which can be tabled for further PRs.

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

changing unsigned int to auto should resolve the warning on windows.

[Karsten1987]

Windows once more: