build: add benchmark build scaffolding#825
Conversation
e922f8f to
0717dac
Compare
|
|
||
| fetchcontent_declare(google_benchmark | ||
| GIT_REPOSITORY https://github.com/google/benchmark.git | ||
| GIT_TAG v1.8.4 |
There was a problem hiding this comment.
nit: should we use the commit SHA like gtest?
There was a problem hiding this comment.
done. I pinned Google Benchmark to the v1.8.4 release commit SHA (a4cf155615c63e019ae549e31703bf367df5b471) and kept the version as an inline comment, matching the gtest style.
0717dac to
b45e7ea
Compare
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for adding this! I just left some comments.
| namespace iceberg { | ||
| namespace { | ||
|
|
||
| void BM_BenchmarkSmoke(benchmark::State& state) { |
There was a problem hiding this comment.
nit: the file name looks odd since it's unrelated to metrics evaluator.
There was a problem hiding this comment.
renamed the smoke benchmark so it isn't tied to metrics evaluator yet.
| OFF | ||
| CACHE BOOL "" FORCE) | ||
|
|
||
| fetchcontent_declare(google_benchmark |
There was a problem hiding this comment.
Should we move this to IcebergThirdpartyToolchain.cmake? I think we should move GTest to there, too.
There was a problem hiding this comment.
I think this PR follows the gtest way ;) Maybe we can change that together, I have the same feeling at the first look.
There was a problem hiding this comment.
opened #829 to track moving both GTest and Google Benchmark into IcebergThirdpartyToolchain.cmake. I’ll keep this PR scoped and follow the current GTest pattern here.
| -DCMAKE_C_COMPILER_LAUNCHER=sccache \ | ||
| -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ | ||
| -DICEBERG_BUILD_SQL_CATALOG=ON \ | ||
| -DICEBERG_BUILD_BENCHMARKS=ON \ |
There was a problem hiding this comment.
Why do we need this but do not change other workflows?
There was a problem hiding this comment.
this is just so the benchmarks get linted. I previously did not have this and cpp-linter failed because the benchmark source wasn’t in compile_commands.json.
There was a problem hiding this comment.
Fair enough. Let's keep it for now.
| CONFIG) | ||
| fetchcontent_makeavailable(google_benchmark) | ||
|
|
||
| add_executable(metrics_evaluator_benchmark metrics_evaluator_benchmark.cc) |
There was a problem hiding this comment.
Should we add EXCLUDE_FROM_ALL so it does not build by default?
There was a problem hiding this comment.
ah I don’t think we need it. I set benchmarks to be off by default (see my CMakeLists.txt change), and when users set ICEBERG_BUILD_BENCHMARKS=ON, the benchmark target should build normally.
There was a problem hiding this comment.
hmm... if we did add EXCLUDE_FROM_ALL then users would have to do
cmake --build build --target benchmark_a benchmark_b benchmark_c benchmark_d
to me a better pattern seems to be:
cmake -S . -B build -DICEBERG_BUILD_BENCHMARKS=ON
cmake --build build
and if user did want to just build specific targets then they can still do:
cmake --build build --target metrics_evaluator_benchmark
what do you think?
b45e7ea to
22aa193
Compare
| -DCMAKE_C_COMPILER_LAUNCHER=sccache \ | ||
| -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ | ||
| -DICEBERG_BUILD_SQL_CATALOG=ON \ | ||
| -DICEBERG_BUILD_BENCHMARKS=ON \ |
There was a problem hiding this comment.
Fair enough. Let's keep it for now.
Summary
Adds an opt-in benchmark build path so future C++ microbenchmarks can live in-tree without affecting normal builds. Benchmarks stay disabled by default, and this PR keeps the executable intentionally small so reviewers can focus on CMake/Meson wiring and Google Benchmark integration.
ICEBERG_BUILD_BENCHMARKSfor CMake and a disabled-by-defaultbenchmarksMeson feature.src/iceberg/benchmarkinto both build systems.Part of #690.
Stack
This is PR 1 of 2.
PR 2 adds the actual metrics evaluator benchmark cases on top of this scaffolding: timothyw553#1.
Test Plan
-DICEBERG_BUILD_BENCHMARKS=ON.metrics_evaluator_benchmarkon this branch.--benchmark_min_time=0.01s.mesonis not installed in this shell.Verification Commands