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

Conversation

PointKernel
Copy link
Member

This PR fixes a bug where both nvbench::main and benchmark_main are trying to insert main() entry points. It creates a new ConfigureNVBench macro to build nvbench benchmarks in a separate directory.

@PointKernel PointKernel added bug Something isn't working 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue non-breaking Non-breaking change labels Jul 6, 2021
@PointKernel PointKernel requested a review from robertmaynard July 6, 2021 17:32
@PointKernel PointKernel self-assigned this Jul 6, 2021
@PointKernel PointKernel requested a review from a team as a code owner July 6, 2021 17:32
@PointKernel PointKernel requested a review from davidwendt July 6, 2021 17:32
@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@c54346e). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 45a4c6c differs from pull request most recent head e0d23b7. Consider uploading reports for the commit e0d23b7 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8662   +/-   ##
===============================================
  Coverage                ?   10.64%           
===============================================
  Files                   ?      109           
  Lines                   ?    18654           
  Branches                ?        0           
===============================================
  Hits                    ?     1985           
  Misses                  ?    16669           
  Partials                ?        0           

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 c54346e...e0d23b7. Read the comment docs.

@@ -56,6 +55,14 @@ function(ConfigureBench 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.

@@ -56,6 +55,14 @@ function(ConfigureBench CMAKE_BENCH_NAME)
PRIVATE cudf_benchmark_common cudf_datagen benchmark::benchmark_main)
endfunction()

function(ConfigureNVBench CMAKE_BENCH_NAME)
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 :)

@PointKernel
Copy link
Member Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit f0f2170 into rapidsai:branch-21.08 Jul 7, 2021
@PointKernel PointKernel deleted the link-nvbench branch July 7, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants