CMake presets for bencher + use in CI - #8287
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41b05883-5e0c-4bde-a945-0de0ceccde8e
|
Max (@maxtropets) is that something we could solve with #7397 and a benchmark preset? I agree that we want the result, but the change itself looks unwieldy. |
Agree, this's [Draft] experiment to measure the gain and judge the worthiness |
Max (maxtropets)
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The changes are localized to build/test wiring and CI scripts, and the dependency aggregation approach is consistent and appears correctly integrated for existing perf/benchmark targets.
Pull request overview
This PR optimizes the benchmarks CI by switching it to a CMake preset-driven build that only builds an explicit subset of targets required by microbenchmarks and perf tests, rather than building the entire project.
Changes:
- Add a
bencherCMake configure/build preset which builds a new aggregate targetccf_bencher. - Introduce
BUILD_TARGETSfor perfadd_e2e_test()registrations and wire these (plus alladd_picobench()executables) as dependencies ofccf_bencher. - Update
bencher.ymland workflow documentation to use the presets and the reduced build.
Custom instructions used:
.github/copilot-instructions.md.github/instructions/reviewing.instructions.md
File summaries
| File | Description |
|---|---|
| CMakePresets.json | Adds bencher configure/build presets to drive a minimal benchmark/perf build via ccf_bencher. |
| CMakeLists.txt | Defines the ccf_bencher aggregate target and annotates perf tests with required build targets. |
| cmake/gersemi_definitions.cmake | Updates add_e2e_test() signature metadata to include BUILD_TARGETS. |
| cmake/common.cmake | Enforces BUILD_TARGETS for perf tests and wires perf/picobench dependencies into ccf_bencher. |
| .gitignore | Ignores CMakeUserPresets.json for local developer overrides. |
| .github/workflows/README.md | Documents how to run bencher workflow builds/tests locally using presets. |
| .github/workflows/bencher.yml | Switches the workflow to cmake --preset bencher and builds only ccf_bencher. |
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "cacheVariables": { | ||
| "CMAKE_BUILD_TYPE": "RelWithDebInfo", | ||
| "WORKER_THREADS": "2" | ||
| } |
| git config --global --add safe.directory /__w/CCF/CCF | ||
| mkdir build | ||
| cmake --preset bencher | ||
| cmake --build --preset bencher |
There was a problem hiding this comment.
Is there a reason to switch to cmake --build instead of (cd build &&) ninja?
There was a problem hiding this comment.
Surely you'd want a single cmake --build command, that's the point of presets no?
| PYTHON_SCRIPT ${CMAKE_SOURCE_DIR}/tests/logging_jwt_locust.py | ||
| LABEL perf | ||
| CONFIGURATIONS perf | ||
| BUILD_TARGETS logging |
There was a problem hiding this comment.
This seems like a roundabout way of defining this? We don't have clean ways to enable/disable individual tests, and yet this is trying to say if this test is enabled, then ccf_bencher must build logging? I think its simpler for now to remove these custom CMake dependencies, and spell out in the preset that bencher builds logging, basic, js_generic. If we add another, we need to remember to add it, but we get clear CI failures if we forget (we try to run a test that depends on a library that hasn't been built). I'd prefer to be even more explicit, and have the tests be their own targets that could be built (and would build their required libs) with the same filtering as when we run them. But CMake doesn't support that, afaict.
|
No consensus on this, closing |
Pending results https://github.com/microsoft/CCF/actions/runs/33876067970
Cuts off 2+ minutes.