Added benchmark test to libstatistics_collector #57

ahcorde · 2020-09-17T10:21:44Z

The only performance-sensitive operations here are those that add measurements. Added benchmark test to libstatistics_collector:

collector
moving_average

@cottsay

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

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

codecov · 2020-09-17T10:55:53Z

Codecov Report

Merging #57 into master will decrease coverage by 0.68%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   25.43%   24.75%   -0.69%     
==========================================
  Files          26       27       +1     
  Lines         798      820      +22     
  Branches      185      185              
==========================================
  Hits          203      203              
- Misses        432      454      +22     
  Partials      163      163

Flag	Coverage Δ
#unittests	`24.75% <ø> (-0.69%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s_collector/test/benchmark/benchmark_iterative.cpp	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25696fa...5ce4a1e. Read the comment docs.

dabonnie · 2020-09-17T16:26:45Z

@ahcorde how can we verify that the performance tests are running as expected?

ahcorde · 2020-09-17T16:36:12Z

To be able to run the benchmark tests you need to compile it with -DAMENT_RUN_PERFORMANCE_TESTS=ON otherwise theses kind of tests will be skipped. Be sure you are working with the rolling distro there are new packages that includes this functionality

dabonnie · 2020-09-17T16:45:41Z

To be able to run the benchmark tests you need to compile it with -DAMENT_RUN_PERFORMANCE_TESTS=ON otherwise theses kind of tests will be skipped. Be sure you are working with the rolling distro there are new packages that includes this functionality

Can you show us the output from running them yourself? I'm guessing we won't see the results in the BuildFarm until merged, correct?

ahcorde · 2020-09-17T16:49:35Z

you should see something like:

5: 2020-09-17 18:46:50                                
5: Running /home/ahcorde/ros2_master/build/libstatistics_collector/benchmark_iterative
5: Run on (8 X 4700 MHz CPU s)
5: CPU Caches:
5:   L1 Data 32 KiB (x8)
5:   L1 Instruction 32 KiB (x8)
5:   L2 Unified 256 KiB (x8)
5:   L3 Unified 12288 KiB (x1)
5: Load Average: 1.02, 0.92, 0.97
5: ***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will incur extra overhead.
5: ***WARNING*** Library was built as DEBUG. Timings may be affected.
5: --------------------------------------------------------------------------------------------------------------
5: Benchmark                                                    Time             CPU   Iterations UserCounters...
5: --------------------------------------------------------------------------------------------------------------
5: PerformanceTest/collector_benchmark                     133777 ns       133628 ns         5181 heap_allocations=386.026u
5: PerformanceTest/moving_average_statistics_benchmark     128240 ns       128095 ns         5464 heap_allocations=366.032u
5: -- run_test.py: return code 0

There is a specific job for running these benchmarks: http://build.ros2.org/view/Rci/job/Rci__benchmark_ubuntu_focal_amd64/

dabonnie · 2020-09-17T20:55:09Z

@ahcorde can you please address the CI failure?

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

…ithub.com/ahcorde/libstatistics_collector into ahcorde/benchmark/libstatistics_collector

benchmark/benchmark_iterative.cpp

cottsay · 2020-09-18T18:02:23Z

benchmark/benchmark_iterative.cpp

+  }
+  st.SetComplexityN(len);
+
+  collector.ClearCurrentMeasurements();


What is the purpose of this? The collector is about to fall out of scope anyway.

This hasn't been addressed yet. If there is a reason that it should be kept, just let me know.

I was trying to use the API properly, anyhow I can remove it.

Removed 6470775

benchmark/benchmark_iterative.cpp

cottsay · 2020-09-18T18:07:50Z

CMakeLists.txt

+  # Give cppcheck hints about macro definitions coming from outside this package
+  get_target_property(ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS
+    performance_test_fixture::performance_test_fixture INTERFACE_INCLUDE_DIRECTORIES)


You shouldn't need both this and the unknownMacro suppression in the code. I don't care which approach is used, but I don't think both are appropriate.

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

cottsay

Thanks for iterating.

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

cottsay

A couple more changes, then I think this one is good to go.

CMakeLists.txt

test/benchmark/benchmark_iterative.cpp

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

* Added benchmark test to libstatistics_collector Signed-off-by: ahcorde <ahcorde@gmail.com> * cppcheck supressed unknown macro warning - macos Signed-off-by: ahcorde <ahcorde@gmail.com> * Reset heap counters Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Remove unknownMacro suppression from CMakeLists.txt Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * moved benchmark test to test/benchmark Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Devin Bonnie <47613035+dabonnie@users.noreply.github.com>

…#59) * Added benchmark test to libstatistics_collector (#57) * Added benchmark test to libstatistics_collector Signed-off-by: ahcorde <ahcorde@gmail.com> * cppcheck supressed unknown macro warning - macos Signed-off-by: ahcorde <ahcorde@gmail.com> * Reset heap counters Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Remove unknownMacro suppression from CMakeLists.txt Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * moved benchmark test to test/benchmark Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Devin Bonnie <47613035+dabonnie@users.noreply.github.com> * Updated Github Actions Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Devin Bonnie <47613035+dabonnie@users.noreply.github.com>

ahcorde requested a review from a team as a code owner September 17, 2020 10:21

ahcorde requested review from dabonnie and jikawa-az and removed request for a team September 17, 2020 10:21

ahcorde force-pushed the ahcorde/benchmark/libstatistics_collector branch from a098fad to da06bed Compare September 17, 2020 10:30

Added benchmark test to libstatistics_collector

d697493

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

ahcorde force-pushed the ahcorde/benchmark/libstatistics_collector branch from da06bed to d697493 Compare September 17, 2020 10:32

Merge branch 'master' into ahcorde/benchmark/libstatistics_collector

2fc139a

ahcorde added 3 commits September 18, 2020 10:12

cppcheck supressed unknown macro warning - macos

2433a62

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

Reset heap counters

ca744fb

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

Merge branch 'ahcorde/benchmark/libstatistics_collector' of https://g…

7b2a05d

…ithub.com/ahcorde/libstatistics_collector into ahcorde/benchmark/libstatistics_collector

dabonnie approved these changes Sep 18, 2020

View reviewed changes

dabonnie requested review from mm318, prajakta-gokhale and zmichaels11 September 18, 2020 17:48

cottsay suggested changes Sep 18, 2020

View reviewed changes

ahcorde added 3 commits September 21, 2020 19:54

Added feedback

72a0c9a

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

Remove unknownMacro suppression from CMakeLists.txt

59ca1b2

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

Added feedback

6470775

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

cottsay approved these changes Sep 22, 2020

View reviewed changes

dabonnie approved these changes Sep 22, 2020

View reviewed changes

jikawa-az approved these changes Sep 22, 2020

View reviewed changes

moved benchmark test to test/benchmark

346eec3

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

cottsay suggested changes Sep 23, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

test/benchmark/benchmark_iterative.cpp Outdated Show resolved Hide resolved

Added feedback

5ce4a1e

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

cottsay approved these changes Sep 24, 2020

View reviewed changes

dabonnie approved these changes Sep 24, 2020

View reviewed changes

dabonnie merged commit a28c862 into ros-tooling:master Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added benchmark test to libstatistics_collector #57

Added benchmark test to libstatistics_collector #57

ahcorde commented Sep 17, 2020

codecov bot commented Sep 17, 2020 •

edited

Loading

dabonnie commented Sep 17, 2020

ahcorde commented Sep 17, 2020

dabonnie commented Sep 17, 2020

ahcorde commented Sep 17, 2020

dabonnie commented Sep 17, 2020 •

edited

Loading

cottsay Sep 18, 2020

cottsay Sep 21, 2020

ahcorde Sep 22, 2020

ahcorde Sep 22, 2020

cottsay Sep 18, 2020

cottsay left a comment

cottsay left a comment

Added benchmark test to libstatistics_collector #57

Added benchmark test to libstatistics_collector #57

Conversation

ahcorde commented Sep 17, 2020

codecov bot commented Sep 17, 2020 • edited Loading

Codecov Report

dabonnie commented Sep 17, 2020

ahcorde commented Sep 17, 2020

dabonnie commented Sep 17, 2020

ahcorde commented Sep 17, 2020

dabonnie commented Sep 17, 2020 • edited Loading

cottsay Sep 18, 2020

Choose a reason for hiding this comment

cottsay Sep 21, 2020

Choose a reason for hiding this comment

ahcorde Sep 22, 2020

Choose a reason for hiding this comment

ahcorde Sep 22, 2020

Choose a reason for hiding this comment

cottsay Sep 18, 2020

Choose a reason for hiding this comment

cottsay left a comment

Choose a reason for hiding this comment

cottsay left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2020 •

edited

Loading

dabonnie commented Sep 17, 2020 •

edited

Loading