Add google benchmark for NormalEstimation #4506

mvieth · 2020-11-11T09:52:47Z

So far, this is in a very early state. I am not sure when this will be review-ready (maybe never). This is mainly to start a discussion.

kunaltyagi · 2020-11-12T03:08:50Z

For ubuntu, you'll have to modify the docker image first

benchmarks/example.cpp

benchmarks/CMakeLists.txt

kunaltyagi

LGTM (IMO it's a good base for future PRs to improve upon)
Nice work @mvieth

@SunBlack could you PTAL at the cmake changes

kunaltyagi · 2020-11-16T02:46:58Z

benchmarks/features.cpp

+  pcl::PointCloud<pcl::Normal>::Ptr cloud_normals (new pcl::PointCloud<pcl::Normal>);
+  for (auto _ : state) {
+    // This code gets timed
+    ne.compute (*cloud_normals);


Does this give satisfactory graphs? Or does the compiler optimize the loop to no-op?

Does this give satisfactory graphs? Or does the compiler optimize the loop to no-op?

I'm not sure if this is the same, but the method is only run for one iteration on the CI:
https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=17871&view=logs&jobId=cd6332f9-d180-53f5-8d9a-3b5d9c3252a2&j=cd6332f9-d180-53f5-8d9a-3b5d9c3252a2&t=7e929b77-dd35-56c3-fbaf-db0044c91b15

The input file is too large for the benchmark to run it multiple times

I just tried with the bun0.pcd and here it runs for about 224 times - so it just seems to be saturated when using 2.5 sec for a benchmark, ie using table_scene_mug_stereo_textured.

The input file is too large for the benchmark to run it multiple times

Saw this after my own test 😄

It is not optimized away, I assume that the for (auto _ : state) has some hidden do-not-optimize-me-magic.
I think it is possible to request it to run more than once, or for a minimum time, if you think once is not enough.

More runs are better since they remove the noise due to ambiguousness of the new cpu architectures (and client sharing on the cloud )

That is true. The number of repetitions can be set for each benchmark individually in the code by calling Repetitions, or for all benchmarks in one executable with --benchmark_repetitions=x. I would suggest the latter, since more flexible (adding it to ARGUMENTS of PCL_ADD_BENCHMARK)

benchmarks/CMakeLists.txt

SunBlack · 2020-11-16T11:51:16Z

benchmarks/CMakeLists.txt

+  set_target_properties(benchmark_${_name} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+  add_custom_target(run_benchmark_${_name} benchmark_${_name} ${PCL_ADD_BENCHMARK_ARGUMENTS})
+  add_dependencies(run_benchmarks run_benchmark_${_name})
+endmacro()


Some notes to the CMake code when I take only a short look at it:

Why it is a macro and not a function?

IDE_FOLDER is missing (we should create a new group for it)

is there a error handling necessary in case the developer is missing a parameter?

I dislike add_executable(benchmark_${_name} ${_name}.cpp) as therefore you have for each benchmark a fixed source name. In general I would prefer to have a benchmark per PCL module (e.g. pcl_benchmark_common). In case someone want to run just one test, he can use --benchmark_filter=...

Thank you for your notes!

what advantages would a function have over a macro? I mainly oriented myself at the existing macros, e.g. PCL_ADD_TEST

error handling: optional IMO. Then it would make sense to implement it for all macros in pcl_targets.cmake

my current plan was to have a cpp file for each module (e.g. features.cpp, common.cpp, filters.cpp, ...), and corresponding executables (benchmark_features, benchmark_common, benchmark_filters, ...). If a module ever has that many benchmarks that it makes sense to split it, we would have to rethink the structure
Please also take another look at the current macro after I applied @larshg's suggestions.

what advantages would a function have over a macro? I mainly oriented myself at the existing macros, e.g. PCL_ADD_TEST

It is like in C++: Content of a macro will be injected where it is called, during function are creating a new scope. So in general: Use function except there is a reason, that you need really a macro

error handling: optional IMO. Then it would make sense to implement it for all macros in pcl_targets.cmake

Indeed it makes sense to introduce it for all. Question here was more: Is everything finde, when LINK_WITH & ARGUMENTS is empty

my current plan was to have a cpp file for each module (e.g. features.cpp, common.cpp, filters.cpp, ...), and corresponding executables (benchmark_features, benchmark_common, benchmark_filters, ...). If a module ever has that many benchmarks that it makes sense to split it, we would have to rethink the structure

In general I would prefer to start in a clean way. So your current benchmark would be within benchmarks/features/normal_3d.cpp. Reason: Normally when someone is adding sth. he just add it and don't think: Oh well, no the file is to big, let's restructure the project. So it is better to start clean (and it is better for the Git history ;-) ).

Please take another look, if I missed any of your suggestions 😃

mvieth · 2020-11-17T10:27:34Z

I considered adding UseRealTime() as discussed with @larshg here, but now that I look at the benchmarks I don't really see how they would be measured differently. What's your take on this?

cmake/pcl_targets.cmake

SunBlack · 2020-11-17T16:02:21Z

cmake/pcl_targets.cmake

+  #Only applies to MSVC
+  if(MSVC)
+    #Requires CMAKE version 3.13.0
+    if(CMAKE_VERSION VERSION_LESS "3.13.0" AND (NOT ArgumentWarningShown))


ArgumentWarningShown will not work as excepted currently. The variable will be expanded within scope of benchmarks/CMakeLists.txt , so it is only available there (and in sub directories of it). So when calling PCL_ADD_BENCHMARK first wihin a subdirectory and the in the main directory, the variable is unset at first time. So you should use a global variable instead for it (I don't recommend CACHE INTERNAL here, as then the warning will be only shown when the CMakeCache will be created and not anytime configure will be called).

Well, it works if its a macro, so its inserted in benchmarks/CMakeLists.txt in which the parent scope will be our main CMakeLists?
Its at least the same solution I used for the unit tests and I'm pretty sure it worked as expected, but my memory might fail me here.

However, if its changed to a function, we probably should change it to a global variable.

Well, it works if its a macro, so its inserted in benchmarks/CMakeLists.txt in which the parent scope will be our main CMakeLists?

No, this our main CMakeLists will not see this variable.

Time for a short explanation.

Example 1

Let's asume we have following CMake files:

benchmark/CMakeList.txt

PCL_ADD_BENCHMARK(features_normal_3d FILES ...) add_subdirectory(another_dir_with_benachmark) add_subdirectory(another_dir_with_benachmark2)

benchmark/another_dir_with_benachmark/CMakeList.txt

PCL_ADD_BENCHMARK(another_benchmark FILES ...)

benchmark/another_dir_with_benachmark2/CMakeList.txt

PCL_ADD_BENCHMARK(another_benchmark2 FILES ...)

This works as you want

Example 1 - Explanation

benchmark/CMakeList.txt

# ArgumentWarningShown is not set until now => PCL_ADD_BENCHMARK will show a warning PCL_ADD_BENCHMARK(features_normal_3d FILES ...) # ArgumentWarningShown is now TRUE after calling PCL_ADD_BENCHMARK add_subdirectory(another_dir_with_benachmark) add_subdirectory(another_dir_with_benachmark2)

benchmark/another_dir_with_benachmark/CMakeList.txt

# ArgumentWarningShown is TRUE like in parent CMakeLists => no warning PCL_ADD_BENCHMARK(another_benchmark FILES ...) #

benchmark/another_dir_with_benachmark2/CMakeList.txt

# ArgumentWarningShown is TRUE like in parent CMakeLists => no warning PCL_ADD_BENCHMARK(another_benchmark2 FILES ...)

Example 2

Now let us a modify the snippet a little bit, by moving first PCL_ADD_BENCHMARK below both calls to add_subdirectory :

benchmark/CMakeList.txt

add_subdirectory(another_dir_with_benachmark) add_subdirectory(another_dir_with_benachmark2) PCL_ADD_BENCHMARK(features_normal_3d FILES ...)

All other files are equal

And now you are getting the warning 3 times. Why? See below

Example 2 - Explanation

benchmark/CMakeList.txt

# ArgumentWarningShown is not set until now add_subdirectory(another_dir_with_benachmark) add_subdirectory(another_dir_with_benachmark2) # ArgumentWarningShown is not set until now, as changes from child scope are not injected into parent scope => PCL_ADD_BENCHMARK will show a warning PCL_ADD_BENCHMARK(features_normal_3d FILES ...)

benchmark/another_dir_with_benachmark/CMakeList.txt

# ArgumentWarningShown is not set until now => PCL_ADD_BENCHMARK will show a warning PCL_ADD_BENCHMARK(another_benchmark FILES ...) # ArgumentWarningShown is now TRUE after calling PCL_ADD_BENCHMARK #

benchmark/another_dir_with_benachmark2/CMakeList.txt

# ArgumentWarningShown is not set until now => PCL_ADD_BENCHMARK will show a warning PCL_ADD_BENCHMARK(another_benchmark2 FILES ...) # ArgumentWarningShown is now TRUE after calling PCL_ADD_BENCHMARK

Conclusion

Calling add_subdirectory is like passing all existing variables to a C++ function as copy and not as reference. The caller will not see the changes of the callee.

Nevertheless it is possible in CMake to modify parent scope:

via global states

using PARENT_SCOPE

PARENT_SCOPE will be usually used in functions, where you pass the variable name and want that the function stores the result into this variable. In other cases I don't recommend using PARENT_SCOPE. We had it in our framework a long time, but it was bug prune, as in case someone is missing one time a call to PARENT_SCOPE, you don't first see why sth. is not working like excepted.

As the variable ArgumentWarningShown is only related to benchmark I would use:

set_target_properties(ArgumentWarningShown run_benchmark PCL_ARGUMENTS_WARNING_SHOWN) if(NOT ArgumentWarningShown) .. set_target_properties(run_benchmark PROPERTIES PCL_ARGUMENTS_WARNING_SHOWN TRUE) endif()

Yes, thats true if its organized differently as first intended, it will stop working. Thanks for the thorough explanation.
@mvieth can you reorganize appropriately?
And maybe change the name ArgumentWarningShown to BenchArgumentWarningShown so it doesn't clash with the one in Unit Tests.

So, macros are basically evil in almost all languages. (:crab:)

@SunBlack I assume you mean get_target_property in the first line, and run_benchmarks, or did I misunderstand something?

cmake/pcl_targets.cmake

Co-authored-by: Lars Glud <larshg@gmail.com>

larshg · 2020-12-20T22:09:26Z

Should this be marked as ready for review soon 😄 ? or maybe just ready for merge 😁

mvieth · 2021-03-30T19:12:32Z

Ok, it's ready for review, but I think before actually merging it, I would disable the benchmarks on the CI again - until we find a way to fix that, it seems to be difficult to compare two runs because they could have been on different machines or under different CPU loads. I think currently the benchmarks are most useful for developers to run on their own computers.

kunaltyagi · 2021-03-31T01:25:45Z

I would disable the benchmarks on the CI again

Seconded

kunaltyagi

rest lgtm

cmake/pcl_targets.cmake

* Add google benchmark for NormalEstimation * Add benchmarks step to ubuntu and windows ci * Suggested changes. * More suggestions. * Add NormalEstimationOMP * Make cmake set lowercase * Change macro to function * Fix unit test->benchmark Co-authored-by: Lars Glud <larshg@gmail.com> * Restructure benchmarks * Add more benchmark variants for NormalEstimation * Correct use of ArgumentWarningShown * Hopefully fix build error on 18.04 GCC * Improve formatting * Fix Windows tests/benchmarks * Do not run benchmarks on CIs Co-authored-by: Lars Glud <larshg@gmail.com>

mvieth force-pushed the benchmarks branch 2 times, most recently from e334f44 to 9125824 Compare November 11, 2020 12:08

mvieth mentioned this pull request Nov 11, 2020

Add benchmarks to PCL #3860

Open

mvieth force-pushed the benchmarks branch 3 times, most recently from 7bb72b4 to 310ba9b Compare November 11, 2020 16:23

larshg reviewed Nov 12, 2020

View reviewed changes

benchmarks/example.cpp Outdated Show resolved Hide resolved

mvieth force-pushed the benchmarks branch 2 times, most recently from 9c8c095 to 88131d4 Compare November 13, 2020 14:02

kunaltyagi reviewed Nov 14, 2020

View reviewed changes

benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved

Add google benchmark for NormalEstimation

e88ed6f

mvieth force-pushed the benchmarks branch from 88131d4 to e88ed6f Compare November 15, 2020 13:25

kunaltyagi previously approved these changes Nov 16, 2020

View reviewed changes

larshg reviewed Nov 16, 2020

View reviewed changes

benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved

Add benchmarks step to ubuntu and windows ci

7eb3a08

SunBlack reviewed Nov 16, 2020

View reviewed changes

larshg added 3 commits November 16, 2020 16:44

Suggested changes.

44df2c4

More suggestions.

b8de429

Add NormalEstimationOMP

51ef38f

SunBlack reviewed Nov 17, 2020

View reviewed changes

cmake/pcl_targets.cmake Outdated Show resolved Hide resolved

SunBlack reviewed Nov 17, 2020

View reviewed changes

mvieth added 2 commits November 17, 2020 20:33

Make cmake set lowercase

3c6a867

Change macro to function

1009577

larshg reviewed Nov 18, 2020

View reviewed changes

cmake/pcl_targets.cmake Outdated Show resolved Hide resolved

mvieth and others added 3 commits November 18, 2020 10:48

Fix unit test->benchmark

40332e6

Co-authored-by: Lars Glud <larshg@gmail.com>

Restructure benchmarks

7a7fe0a

Add more benchmark variants for NormalEstimation

4f06cc0

mvieth mentioned this pull request Nov 25, 2020

Faster organized search #4496

Merged

mvieth force-pushed the benchmarks branch from 10b8d02 to ecfafb9 Compare November 27, 2020 13:43

Correct use of ArgumentWarningShown

d4db768

mvieth force-pushed the benchmarks branch from ecfafb9 to d4db768 Compare November 27, 2020 14:14

larshg mentioned this pull request Mar 29, 2021

Speeding up GPU clustering using smarter download strategy and memory allocations #4677

Merged

mvieth added 2 commits March 30, 2021 10:07

Merge branch 'master' into benchmarks

70c37ba

Hopefully fix build error on 18.04 GCC

f2740ce

mvieth marked this pull request as ready for review March 30, 2021 19:06

Merge branch 'master' into benchmarks

6ed4d35

mvieth dismissed kunaltyagi’s stale review via 6ed4d35 June 25, 2021 19:22

mvieth mentioned this pull request Jun 26, 2021

Install google benchmark on windows docker #4815

Merged

mvieth added 3 commits June 27, 2021 10:19

Improve formatting

9072aee

Fix Windows tests/benchmarks

a8f9511

Do not run benchmarks on CIs

9263ed9

mvieth requested review from kunaltyagi and larshg June 28, 2021 12:19

larshg approved these changes Jun 28, 2021

View reviewed changes

kunaltyagi reviewed Jun 28, 2021

View reviewed changes

cmake/pcl_targets.cmake Show resolved Hide resolved

kunaltyagi approved these changes Jun 28, 2021

View reviewed changes

kunaltyagi merged commit 2d7ebf1 into PointCloudLibrary:master Jun 28, 2021

mvieth deleted the benchmarks branch August 5, 2021 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add google benchmark for NormalEstimation #4506

Add google benchmark for NormalEstimation #4506

mvieth commented Nov 11, 2020

kunaltyagi commented Nov 12, 2020

kunaltyagi left a comment

kunaltyagi Nov 16, 2020

larshg Nov 16, 2020

kunaltyagi Nov 16, 2020

larshg Nov 16, 2020

larshg Nov 16, 2020

mvieth Nov 18, 2020

kunaltyagi Nov 24, 2020

mvieth Nov 27, 2020

SunBlack Nov 16, 2020 •

edited

Loading

mvieth Nov 17, 2020

SunBlack Nov 17, 2020

mvieth Nov 27, 2020

mvieth commented Nov 17, 2020

SunBlack Nov 17, 2020

larshg Nov 18, 2020

SunBlack Nov 19, 2020 •

edited

Loading

larshg Nov 25, 2020

kunaltyagi Nov 26, 2020

mvieth Nov 27, 2020

larshg commented Dec 20, 2020

mvieth commented Mar 30, 2021

kunaltyagi commented Mar 31, 2021

kunaltyagi left a comment

Add google benchmark for NormalEstimation #4506

Add google benchmark for NormalEstimation #4506

Conversation

mvieth commented Nov 11, 2020

kunaltyagi commented Nov 12, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunBlack Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvieth commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunBlack Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Example 1

Example 1 - Explanation

Example 2

Example 2 - Explanation

Conclusion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larshg commented Dec 20, 2020

mvieth commented Mar 30, 2021

kunaltyagi commented Mar 31, 2021

kunaltyagi left a comment

Choose a reason for hiding this comment

SunBlack Nov 16, 2020 •

edited

Loading

SunBlack Nov 19, 2020 •

edited

Loading