Updating performance writer #570

adamdbrw · 2020-11-24T15:17:00Z

This is still the "clunky" script. While the refactoring of performance writer is scheduled, this PR contains several important updates that make the performance benchmarking packages usable with recent changes:

uses --storage-config-file option with two examples (optimized and non-optimized). This enables testing of other pragma settings easily at will.
message lost statistic is now read from metadata yaml file. This is more correct and also makes performance benchmarking ready for cache double-buffers PR 546.
removed performance-affecting outputs (writing '.' and 'X' on writes and misses affected performance, especially with multiple instances & lots of small messages).
supports bag splitting in benchmarking by exposing max_bagfile_size parameter to the script.

This PR does not replace the necessary work to be done in performance packages (replacing the script with launch files, expanding parametrization), but still serves as a strict upgrade over the current state. Some of these changes should have been a part of earlier PRs.

- utilizes --storage-config-file with two examples - message reading from metadata yaml file (ready for cache double-buffers PR 546) - removed performance-affecting outputs - supports bag splitting in benchmarking Signed-off-by: Adam Dabrowski <[email protected]>

adamdbrw · 2020-11-25T15:22:00Z

@mjeronimo this PR has some updates to benchmarking, could you take a look?

mjeronimo

Looks good to me. One minor grammatical mistake/typo.

rosbag2_performance/rosbag2_performance_writer_benchmarking/src/writer_benchmark.cpp

Signed-off-by: Adam Dabrowski <[email protected]>

adamdbrw · 2020-11-30T18:22:06Z

@mjeronimo shall we run CI and merge this if all turns out OK?

mjeronimo · 2020-11-30T18:29:47Z

Yes. Launched the CI:

Linux
Linux-aarch64
macOS
Windows

mjeronimo · 2020-11-30T22:57:11Z

Restart the MacOS build:

pijaro · 2020-12-01T14:27:13Z

I've changed how yaml is included in writer_benchmark.cpp for Windows. We can rerun windows CI now and see if everything is compiling fine.

mjeronimo · 2020-12-01T16:45:10Z

Restarted the Windows build:

All warnings are present in the nightly build for Windows.

mjeronimo · 2020-12-01T17:10:11Z

Background information on the pragmas added:

The class YAML::Node is exported in a DLL, but includes std C++ objects in its interface, such as:
const std::string& Scalar() const;

mjeronimo · 2020-12-01T23:26:17Z

Re-ran the CI since there was a code change:

Linux
Linux-aarch64
macOS
Windows

The Windows build has the expected unrelated warnings.

mjeronimo · 2020-12-01T23:28:22Z

@emersonknapp Any comments before merging?

adamdbrw · 2020-12-07T17:27:30Z

@emersonknapp should we proceed to merge? (after resolving the conflict)

emersonknapp

Yes, this LGTM after conflict resolution and green CI (the windows warnings are fixed now)

Signed-off-by: Adam Dabrowski <[email protected]>

…se it is empty (the default) Signed-off-by: Adam Dabrowski <[email protected]>

adamdbrw · 2020-12-14T12:27:03Z

The conflicting commit, Compress bag files in separate threads, actually introduced some errors in rosbag2_performance_writer_benchmarking:

The benchmarking script was broken for empty (default) value of compression_format
The writer_ variable was incorrectly overwritten, see here

I have corrected these errors, however this also means that compression was likely not tested with the performance packages. @emersonknapp, @pjreed.

This fact is not blocking this PR AFAIK.

pjreed · 2020-12-14T16:54:35Z

Good catch. I think that issue with the variable being overwritten was probably introduced as a result of merging to fix conflicts, as I'm pretty sure when I was doing performance testing that I observed compression running. Anyway, this change looks good to me.

mjeronimo · 2020-12-14T17:42:38Z

Re-runing CI after latest change

Linux
Linux-aarch64
macOS
Windows

A couple items to fix: A new warning to fix in MacOS and an issue with the Windows build

Signed-off-by: Adam Dabrowski <[email protected]>

adamdbrw · 2020-12-15T12:45:02Z

The warning on MacOS is concerned with an unused private member that was added in #506 - and should have been causing warnings before - I don't know why this happens only now, perhaps a compiler update or some settings changed? Anyway, an easy fix (see this line).

The windows fail is the usual Yaml stuff - should have known by now. Please note that the benchmarking package is not yet that usable on windows - the main utility script is not portable.

@mjeronimo could we restart the CI and merge if it passes without issues?

mjeronimo · 2020-12-15T20:24:12Z

Linux
Linux-aarch64
macOS
Windows

Restart the macOS build

* Updating performance writer - utilizes --storage-config-file with two examples - message reading from metadata yaml file (ready for cache double-buffers PR 546) - removed performance-affecting outputs - supports bag splitting in benchmarking Signed-off-by: Adam Dabrowski <[email protected]> * typo fix Signed-off-by: Adam Dabrowski <[email protected]> * corrected compression so that it doesn't break the command line in case it is empty (the default) Signed-off-by: Adam Dabrowski <[email protected]> * address CI warns/fails on MacOS and Windows Signed-off-by: Adam Dabrowski <[email protected]>

adamdbrw requested a review from a team as a code owner November 24, 2020 15:17

adamdbrw requested review from emersonknapp and Karsten1987 and removed request for a team November 24, 2020 15:17

mjeronimo reviewed Nov 25, 2020

View reviewed changes

rosbag2_performance/rosbag2_performance_writer_benchmarking/src/writer_benchmark.cpp Outdated Show resolved Hide resolved

adamdbrw added 2 commits November 26, 2020 12:51

typo fix

be206d0

Signed-off-by: Adam Dabrowski <[email protected]>

Merge branch 'master' into performance_writer_benchmarking_update

ccb96b7

Signed-off-by: Adam Dabrowski <[email protected]>

mjeronimo approved these changes Nov 30, 2020

View reviewed changes

emersonknapp approved these changes Dec 7, 2020

View reviewed changes

adamdbrw added 2 commits December 14, 2020 13:15

Merge branch 'master' into performance_writer_benchmarking_update

c479f6e

Signed-off-by: Adam Dabrowski <[email protected]>

corrected compression so that it doesn't break the command line in ca…

95f1351

…se it is empty (the default) Signed-off-by: Adam Dabrowski <[email protected]>

adamdbrw force-pushed the performance_writer_benchmarking_update branch from 82d4f1b to 95f1351 Compare December 14, 2020 12:17

address CI warns/fails on MacOS and Windows

c01a783

Signed-off-by: Adam Dabrowski <[email protected]>

mjeronimo merged commit 0bb6ab8 into master Dec 16, 2020

delete-merged-branch bot deleted the performance_writer_benchmarking_update branch December 16, 2020 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating performance writer #570

Updating performance writer #570

adamdbrw commented Nov 24, 2020 •

edited

Loading

adamdbrw commented Nov 25, 2020

mjeronimo left a comment

adamdbrw commented Nov 30, 2020

mjeronimo commented Nov 30, 2020 •

edited

Loading

mjeronimo commented Nov 30, 2020

pijaro commented Dec 1, 2020

mjeronimo commented Dec 1, 2020 •

edited

Loading

mjeronimo commented Dec 1, 2020

mjeronimo commented Dec 1, 2020

mjeronimo commented Dec 1, 2020

adamdbrw commented Dec 7, 2020

emersonknapp left a comment

adamdbrw commented Dec 14, 2020

pjreed commented Dec 14, 2020

mjeronimo commented Dec 14, 2020 •

edited

Loading

adamdbrw commented Dec 15, 2020 •

edited

Loading

mjeronimo commented Dec 15, 2020 •

edited

Loading

Updating performance writer #570

Updating performance writer #570

Conversation

adamdbrw commented Nov 24, 2020 • edited Loading

adamdbrw commented Nov 25, 2020

mjeronimo left a comment

Choose a reason for hiding this comment

adamdbrw commented Nov 30, 2020

mjeronimo commented Nov 30, 2020 • edited Loading

mjeronimo commented Nov 30, 2020

pijaro commented Dec 1, 2020

mjeronimo commented Dec 1, 2020 • edited Loading

mjeronimo commented Dec 1, 2020

mjeronimo commented Dec 1, 2020

mjeronimo commented Dec 1, 2020

adamdbrw commented Dec 7, 2020

emersonknapp left a comment

Choose a reason for hiding this comment

adamdbrw commented Dec 14, 2020

pjreed commented Dec 14, 2020

mjeronimo commented Dec 14, 2020 • edited Loading

adamdbrw commented Dec 15, 2020 • edited Loading

mjeronimo commented Dec 15, 2020 • edited Loading

adamdbrw commented Nov 24, 2020 •

edited

Loading

mjeronimo commented Nov 30, 2020 •

edited

Loading

mjeronimo commented Dec 1, 2020 •

edited

Loading

mjeronimo commented Dec 14, 2020 •

edited

Loading

adamdbrw commented Dec 15, 2020 •

edited

Loading

mjeronimo commented Dec 15, 2020 •

edited

Loading