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

Add google benchmark for NormalEstimation #4506

Merged
merged 17 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .ci/azure-pipelines/build/macos.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ steps:
# find the commit hash on a quick non-forced update too
fetchDepth: 10
- script: |
brew install cmake pkg-config boost eigen flann glew libusb qhull vtk glew qt5 libpcap libomp brewsci/science/openni
brew install cmake pkg-config boost eigen flann glew libusb qhull vtk glew qt5 libpcap libomp google-benchmark brewsci/science/openni
git clone https://github.com/abseil/googletest.git $GOOGLE_TEST_DIR # the official endpoint changed to abseil/googletest
cd $GOOGLE_TEST_DIR && git checkout release-1.8.1
displayName: 'Install Dependencies'
Expand All @@ -19,6 +19,7 @@ steps:
-DPCL_ONLY_CORE_POINT_TYPES=ON \
-DBUILD_simulation=ON \
-DBUILD_global_tests=ON \
-DBUILD_benchmarks=ON \
-DBUILD_examples=ON \
-DBUILD_tools=ON \
-DBUILD_apps=ON \
Expand All @@ -34,6 +35,8 @@ steps:
displayName: 'Build Library'
- script: cd $BUILD_DIR && cmake --build . -- tests
displayName: 'Run Unit Tests'
- script: cd $BUILD_DIR && cmake --build . -- run_benchmarks
displayName: 'Run Benchmarks'
- task: PublishTestResults@2
inputs:
testResultsFormat: 'CTest'
Expand Down
4 changes: 4 additions & 0 deletions .ci/azure-pipelines/build/ubuntu.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ steps:
-DBUILD_simulation=ON \
-DBUILD_surface_on_nurbs=ON \
-DBUILD_global_tests=ON \
-DBUILD_benchmarks=ON \
-DBUILD_examples=ON \
-DBUILD_tools=ON \
-DBUILD_apps=ON \
Expand All @@ -36,6 +37,9 @@ steps:
- script: |
cd $BUILD_DIR && cmake --build . -- tests
displayName: 'Run Unit Tests'
- script: |
cd $BUILD_DIR && cmake --build . -- run_benchmarks
displayName: 'Run Benchmarks'
- task: PublishTestResults@2
inputs:
testResultsFormat: 'CTest'
Expand Down
7 changes: 6 additions & 1 deletion .ci/azure-pipelines/build/windows.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ steps:
- pwsh: Get-PSDrive
displayName: "Check free space"
- script: |
vcpkg.exe install eigen3 flann gtest qhull --triplet %PLATFORM%-windows && vcpkg.exe list
vcpkg.exe install eigen3 flann gtest qhull benchmark --triplet %PLATFORM%-windows && vcpkg.exe list
displayName: 'Install C++ Dependencies Via Vcpkg'
- script: |
mkdir %BUILD_DIR% && cd %BUILD_DIR%
Expand All @@ -24,6 +24,7 @@ steps:
-DPCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32=ON ^
-DPCL_BUILD_WITH_QHULL_DYNAMIC_LINKING_WIN32=ON ^
-DBUILD_global_tests=ON ^
-DBUILD_benchmarks=ON ^
-DBUILD_tools=OFF ^
-DBUILD_surface_on_nurbs=ON
displayName: 'CMake Configuration'
Expand All @@ -36,6 +37,10 @@ steps:
cd %BUILD_DIR% && cmake --build . --target tests --config %CONFIGURATION%
displayName: 'Run Unit Tests'
workingDirectory: 'c:'
- script: |
cd %BUILD_DIR% && cmake --build . --target run_benchmarks --config %CONFIGURATION%
displayName: 'Run Benchmarks'
workingDirectory: 'c:'
- task: PublishTestResults@2
inputs:
testResultsFormat: 'CTest'
Expand Down
16 changes: 16 additions & 0 deletions benchmarks/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
set(SUBSYS_NAME benchmarks)
set(SUBSYS_DESC "Point cloud library benchmarks")
set(SUBSYS_DEPS common features search kdtree io)
set(DEFAULT OFF)
set(build TRUE)
set(REASON "Disabled by default")
PCL_SUBSYS_OPTION(build "${SUBSYS_NAME}" "${SUBSYS_DESC}" ${DEFAULT} "${REASON}")
PCL_SUBSYS_DEPEND(build "${SUBSYS_NAME}" DEPS ${SUBSYS_DEPS})
if(NOT build)
return()
endif()

find_package(benchmark REQUIRED)
add_custom_target(run_benchmarks)

PCL_ADD_BENCHMARK(features LINK_WITH pcl_io pcl_search pcl_features ARGUMENTS "${PCL_SOURCE_DIR}/test/table_scene_mug_stereo_textured.pcd")
51 changes: 51 additions & 0 deletions benchmarks/features.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#include <benchmark/benchmark.h>
#include <pcl/features/normal_3d.h> // for NormalEstimation
#ifdef _OPENMP
#include <pcl/features/normal_3d_omp.h> // for NormalEstimationOMP
#endif
#include <pcl/io/pcd_io.h> // for PCDReader

static void BM_NormalEstimation(benchmark::State& state, const std::string& file) {
// Perform setup here
pcl::PointCloud<pcl::PointXYZ>::Ptr cloud (new pcl::PointCloud<pcl::PointXYZ>);
pcl::PCDReader reader;
reader.read (file, *cloud);
pcl::NormalEstimation<pcl::PointXYZ, pcl::Normal> ne;
ne.setInputCloud (cloud);
ne.setKSearch (100);
pcl::PointCloud<pcl::Normal>::Ptr cloud_normals (new pcl::PointCloud<pcl::Normal>);
for (auto _ : state) {
// This code gets timed
ne.compute (*cloud_normals);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Saw this after my own test 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

}
}
#ifdef _OPENMP
static void BM_NormalEstimationOMP(benchmark::State& state, const std::string& file) {
// Perform setup here
pcl::PointCloud<pcl::PointXYZ>::Ptr cloud(new pcl::PointCloud<pcl::PointXYZ>);
pcl::PCDReader reader;
reader.read(file, *cloud);
pcl::NormalEstimationOMP<pcl::PointXYZ, pcl::Normal> ne;
ne.setInputCloud(cloud);
ne.setKSearch(100);
pcl::PointCloud<pcl::Normal>::Ptr cloud_normals(new pcl::PointCloud<pcl::Normal>);
for (auto _ : state) {
// This code gets timed
ne.compute(*cloud_normals);
}
}
#endif

int main(int argc, char** argv) {
if (argc < 2)
{
std::cerr << "No test file given. Please download `table_scene_mug_stereo_textured.pcd` and pass its path to the test." << std::endl;
return (-1);
}
benchmark::RegisterBenchmark("BM_NormalEstimation", BM_NormalEstimation, argv[1])->Unit(benchmark::kMillisecond);
#ifdef _OPENMP
benchmark::RegisterBenchmark("BM_NormalEstimationOMP", BM_NormalEstimationOMP, argv[1])->Unit(benchmark::kMillisecond);
#endif
benchmark::Initialize(&argc, argv);
benchmark::RunSpecifiedBenchmarks();
}
39 changes: 39 additions & 0 deletions cmake/pcl_targets.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,45 @@ macro(PCL_ADD_TEST _name _exename)
add_dependencies(tests ${_exename})
endmacro()

###############################################################################
# Add a benchmark target.
# _name The benchmark name.
# _exename The exe name.
# ARGN :
# ARGUMENTS Arguments for benchmark executable
# LINK_WITH link benchmark executable with libraries
macro(PCL_ADD_BENCHMARK _name)
set(options)
set(oneValueArgs)
set(multiValueArgs ARGUMENTS LINK_WITH)
cmake_parse_arguments(PCL_ADD_BENCHMARK "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})

add_executable(benchmark_${_name} ${_name}.cpp)
set_target_properties(benchmark_${_name} PROPERTIES FOLDER "Benchmarks")
target_link_libraries(benchmark_${_name} benchmark::benchmark ${PCL_ADD_BENCHMARK_LINK_WITH})
set_target_properties(benchmark_${_name} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})

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

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@SunBlack SunBlack Nov 19, 2020

Choose a reason for hiding this comment

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

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()

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

message(WARNING "Arguments for unit test projects are not added - this requires at least CMake 3.13. Can be added manually in \"Project settings -> Debugging -> Command arguments\"")
mvieth marked this conversation as resolved.
Show resolved Hide resolved
SET (ArgumentWarningShown TRUE PARENT_SCOPE)
larshg marked this conversation as resolved.
Show resolved Hide resolved
else()
#Only add if there are arguments to test
if(PCL_ADD_BENCHMARK_ARGUMENTS)
kunaltyagi marked this conversation as resolved.
Show resolved Hide resolved
string (REPLACE ";" " " PCL_ADD_BENCHMARK_ARGUMENTS_STR "${PCL_ADD_BENCHMARK_ARGUMENTS}")
set_target_properties(benchmark_${_name} PROPERTIES VS_DEBUGGER_COMMAND_ARGUMENTS ${PCL_ADD_BENCHMARK_ARGUMENTS_STR})
endif()
endif()
endif()

add_custom_target(run_benchmark_${_name} benchmark_${_name} ${PCL_ADD_BENCHMARK_ARGUMENTS})
set_target_properties(run_benchmark_${_name} PROPERTIES FOLDER "Benchmarks")

add_dependencies(run_benchmarks run_benchmark_${_name})
endmacro()

###############################################################################
# Add an example target.
# _name The example name.
Expand Down