Update benchmarking guide to use NVBench. #10093
Conversation
bdice
changed the title
| `cudf/cpp/src/copying/scatter.cu` has benchmarks in | ||
| `cudf/cpp/benchmarks/copying/scatter_benchmark.cu`. | ||
| `cpp/src/copying/scatter.cu` has benchmarks in | ||
| `cpp/benchmarks/copying/scatter_benchmark.cu`. |
There was a problem hiding this comment.
I've been naming nvbench files with _nvbench.cu/cpp suffix as opposed to _benchmark.cu/cpp in gbench and executable with <feature>_NVBENCH (instead of <feature>_BENCH) to distinguish them from gbench. Not sure it's the right way to go for all nvbench benchmarks though.
There was a problem hiding this comment.
I would prefer no suffix on the source files. As nvbench becomes the primary benchmark it becomes just noise.
There was a problem hiding this comment.
@robertmaynard Does "no suffix" mean "remove _benchmark" as well?
I find _benchmark helpful for finding files by name, but I agree that _nvbench is too specific.
There was a problem hiding this comment.
I recall I decided to do so because of CMake:
ConfigureBenchto build_benchmark.cu/cppConfigureNVBenchto build_nvbench.cu/cpp
There was a problem hiding this comment.
@robertmaynard Does "no suffix" mean "remove
_benchmark" as well?I find
_benchmarkhelpful for finding files by name, but I agree that_nvbenchis too specific.
I was thinking of tool specific suffix. So benchmark seems okay to me. The purist in me says that we have benchmarks in the path to the file already so no future distinction is required ( we don't use scatter_header.h ).
I recall I decided to do so because of
CMake:
ConfigureBenchto build_benchmark.cu/cppConfigureNVBenchto build_nvbench.cu/cpp
ConfigureBench and ConfigureNVBench are given a set of souce files to compile, and don't care how those files are named. So they would have no problem with scatter_badly_named.cu :)
The primary issue is that we have join_nvbench.cu and join_benchmark.cu, where the nvbench version tests the left_join, full_join, and inner_join algorithms, and google benchmarks tests the semi and anti joins. If we had gone with file names that matched the algorithms ( anti_join_benchmark.cu, left_join_benchmark.cu ) we wouldn't need to have anything end with _nvbench.
There was a problem hiding this comment.
So do we agree that no change is needed in the guide? (Perhaps some changes are needed to rename _nvbench.cu/cpp files.)
There was a problem hiding this comment.
I prefer to ideally lose suffixes.
Once our benchmarks become large enough to need to split across many files, we might need to add suffixes based on context of that benchmark. At that point, we would prefer to lose _nvbench, _benchmark suffixes because of long filenames if we had it.
There was a problem hiding this comment.
@karthikeyann I can see the argument to remove _benchmark. However, that wouldn't align with the current state of the benchmarks. I'd like to keep this PR focused on the change to "use nvbench" and that change can be done at a later point.
There was a problem hiding this comment.
Just make sure when someone goes through and removes _benchmark from every benchmark source file, they also update the guide to not use the suffix. (If it were me, I would change the example in the guide now...)
There was a problem hiding this comment.
@harrism I'll merge this PR and do that now. 👍
|
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #10093 +/- ##
================================================
+ Coverage 10.37% 10.42% +0.04%
================================================
Files 119 119
Lines 20149 20607 +458
================================================
+ Hits 2091 2148 +57
- Misses 18058 18459 +401
Continue to review full report at Codecov.
|
|
@gpucibot merge |
|
I have addressed follow-up comments about removing |
This PR addresses suggestions from @robertmaynard, @karthikeyann, and @harrism in #10093 to remove the suffix `_benchmark` from the name of the benchmark files. The developer guide has been updated to reflect this change. I left a few shared files with the word "benchmark" in them, such as [cpp/benchmarks/fixture/benchmark_fixture.hpp](https://github.com/rapidsai/cudf/blob/cfb6cbe6a8e4b01d7ee28fbe3212b5f2eb275167/cpp/benchmarks/fixture/benchmark_fixture.hpp) and [cpp/benchmarks/fixture/templated_benchmark_fixture.hpp](https://github.com/rapidsai/cudf/blob/cfb6cbe6a8e4b01d7ee28fbe3212b5f2eb275167/cpp/benchmarks/fixture/templated_benchmark_fixture.hpp). I built the benchmarks locally to ensure that nothing was broken (I'm noting this because I recall that our CI does not build the benchmarks). Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Robert Maynard (https://github.com/robertmaynard) - Karthikeyan (https://github.com/karthikeyann) - Nghia Truong (https://github.com/ttnghia) URL: #10112
This PR relates to #7960 and indicates that new benchmarks should be written using NVBench. (This PR does not resolve that issue because it does not transition any existing benchmarks to nvbench, it is only an update in the developer guide.)