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 ConfigureNVBench to avoid concurrent main() entry points #8662

Merged
merged 3 commits into from
Jul 7, 2021
Merged
Changes from all 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
11 changes: 9 additions & 2 deletions cpp/benchmarks/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ target_link_libraries(cudf_datagen
GTest::gmock_main
GTest::gtest_main
benchmark::benchmark
nvbench::main
Threads::Threads
cudf)

Expand All @@ -51,11 +50,19 @@ target_link_libraries(cudf_benchmark_common PRIVATE cudf_datagen)
function(ConfigureBench CMAKE_BENCH_NAME)
add_executable(${CMAKE_BENCH_NAME} ${ARGN})
set_target_properties(${CMAKE_BENCH_NAME}
PROPERTIES RUNTIME_OUTPUT_DIRECTORY "$<BUILD_INTERFACE:${CUDF_BINARY_DIR}/gbenchmarks>")
PROPERTIES RUNTIME_OUTPUT_DIRECTORY "$<BUILD_INTERFACE:${CUDF_BINARY_DIR}/benchmarks>")
target_link_libraries(${CMAKE_BENCH_NAME}
PRIVATE cudf_benchmark_common cudf_datagen benchmark::benchmark_main)
endfunction()

function(ConfigureNVBench CMAKE_BENCH_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea behind this PR is to finally replace all current benchmarks (which use google benchmark) by using nvbench. I'm working on the first use case of this macro so it's not used yet.

add_executable(${CMAKE_BENCH_NAME} ${ARGN})
set_target_properties(${CMAKE_BENCH_NAME}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we want to place benchmarks in different directories based on if they used gbench or nvbench. To the person running the benchmark it is an implementation detail, and they would expect all benchmarks to be located in the same folder.

For that reason I think placing all the benchmarks in either the gbenchmark or benchmark folder seems more reasonable to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I hesitate on this different directories idea also. The gbenchmark naming basically implies that google benchmark is applied. So my only concern is that creating a nvbench code in gbenchmark would be misleading.

Copy link
Contributor

Choose a reason for hiding this comment

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

So my only concern is that creating a nvbench code in gbenchmark would be misleading.

I think most developers won't remember what benchmarks use nvbench, and will be confused on the split.

This might be the perfect time to have all benchmarks in <build_dir>/benchmarks :)

PROPERTIES RUNTIME_OUTPUT_DIRECTORY "$<BUILD_INTERFACE:${CUDF_BINARY_DIR}/benchmarks>")
target_link_libraries(${CMAKE_BENCH_NAME}
PRIVATE cudf_benchmark_common cudf_datagen nvbench::main)
endfunction()

###################################################################################################
# - column benchmarks -----------------------------------------------------------------------------
ConfigureBench(COLUMN_CONCAT_BENCH column/concatenate_benchmark.cpp)
Expand Down