perf(cmake): analyse a package's translation units in parallel under clang-tidy - #588
Merged
Conversation
…clang-tidy ament_clang_tidy defaults to --jobs 1 and the wrapper passed neither JOBS nor ament_cmake_clang_tidy_JOBS, so the full gate walked one translation unit at a time while the rest of the machine idled. CTest parallelism did not compensate: each package registers a single clang_tidy test, so -j only overlaps packages and the largest one still set the wall clock. Pass a job count through the wrapper, defaulting to the host core count, and run the tidy preset with one CTest job. The parallelism now lives inside each package test; keeping both levels would multiply peak memory by the number of packages, and a clang-tidy process on this codebase holds roughly half a gigabyte. Closes #586
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The updated README inaccurately states this clang-tidy CTest path is “on in CI”, but CI’s quality.yml clang-tidy job configures with -DENABLE_CLANG_TIDY=OFF and uses run-clang-tidy instead.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Improves the developer-local “full clang-tidy gate” by enabling per-package parallel analysis (instead of one translation unit at a time), while avoiding unsafe memory amplification from also running multiple packages’ clang-tidy tests concurrently.
Changes:
- Add
ROS2_MEDKIT_CLANG_TIDY_JOBS(default: host core count) and a per-packageJOBSoverride toros2_medkit_clang_tidy(), forwarding the value toament_clang_tidy. - Update
scripts/test.sh tidyto run theclang_tidyCTest target with-j 1so parallelism lives inside each package’s test. - Document the new knobs and the intended parallelism model in
ros2_medkit_cmakeREADME.
File summaries
| File | Description |
|---|---|
| src/ros2_medkit_cmake/README.md | Documents the clang-tidy CTest target behavior and new per-package job configuration. |
| src/ros2_medkit_cmake/cmake/ROS2MedkitLinting.cmake | Adds job-count configuration and forwards it to ament_clang_tidy to enable parallel TU analysis. |
| scripts/test.sh | Adjusts the tidy preset to avoid CTest-level parallelism when running clang-tidy. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
mfaferek93
reviewed
Aug 5, 2026
Three problems with the parallel clang-tidy gate as it stood. The default job count was the host core count, which is unbounded in memory. Measured on ros2_medkit_gateway, memory scales linearly at roughly 1.2 GiB per job while wall clock does not: jobs=2 1078 s 2.6 GiB peak resident jobs=4 595 s 4.7 GiB jobs=8 359 s 9.4 GiB jobs=16 272 s 17.4 GiB Size the default so one package fits an 8 GB machine and it lands at min(host cores, 2). That is deliberately the slow end of the curve; the alternative is a default that only works on the largest machine anyone here has, and 17.4 GiB for a single package is not something we can assume. Add the switch that makes the low default liveable. The count is baked into the CTest command at configure time, so ./scripts/test.sh tidy --jobs <n> reconfigures the clang-tidy packages first - about three seconds each, no recompilation - then runs. The value persists in the CMake cache, so every tidy run prints the count in effect rather than leaving it as invisible state. The tidy preset did not actually serialise packages. It passed --ctest-args -j 1, but that flag cannot do it: a participating package registers exactly one clang_tidy test and colcon runs a separate ctest per package, so -j has nothing to overlap. What schedules packages is colcon's --parallel-workers, which COMMON_ARGS set to nproc - so every package could analyse concurrently while each one spawned its own jobs, the multiplication the comment claimed to prevent. Pin --parallel-workers 1 after the caller's arguments. colcon keeps recognising its own options after --ctest-args and the option is a plain argparse store, so the last occurrence wins; placed earlier, a caller's own --parallel-workers silently undid the guard. The preset reports when it ignores such an override. Confirmed against colcon's scheduler: package Starting/Finished pairs no longer interleave, with or without a trailing override. Fail the run when a clang-tidy process dies. ament_clang_tidy derives its exit status from the warnings it parsed, so a process killed by the OOM reaper contributes nothing and the test passes as if the package were clean - the worst way to lose coverage. run_test.py merges the child's stderr into the per-test log, so the tidy preset scans those logs for the failure marker and fails, naming what went unanalysed. Correct the CI description where it contradicted itself. The module table and the linting section both called clang-tidy "mandatory in CI", and the design doc said the same, but quality.yml configures with -DENABLE_CLANG_TIDY=OFF and runs run-clang-tidy over the compilation database, so the CTest target is a local gate only. The 0.4.0 changelog entry keeps that wording: CI only switched in April, after that release. Also qualify "each package" - packages excluding ament_cmake_clang_tidy register no such test - and note the ProcessorCount fallback to 1.
mfaferek93
approved these changes
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
The full clang-tidy gate analysed one translation unit at a time regardless of how many cores the machine had.
ament_clang_tidydefaults to--jobs 1, andros2_medkit_clang_tidy()passed neither aJOBSargument nor theament_cmake_clang_tidy_JOBSvariable the upstream macro also honours. CTest-level parallelism did not compensate, because each package registers a singleclang_tidytest, so-jonly overlaps packages while the largest one sets the wall clock on its own.This passes a job count through the wrapper, defaulting to the host core count, and runs the
tidypreset with one CTest job. The parallelism now lives inside each package test; keeping both levels would multiply peak memory by the number of packages, and aclang-tidyprocess on this codebase holds roughly half a gigabyte.Two paths are deliberately unchanged:
quality.ymldrivesrun-clang-tidywithclang-tidy-cache, and analyses only changed files on pull requests.scripts/clang-tidy-diff.shloops serially, but pre-commit already invokes it with several file batches at once, so it is not the bottleneck.Issue
Type
Build tooling / developer ergonomics.
Testing
Measured on the
ros2_medkit_gatewaypackage (244 entries in its compile database), 16-core machine, same worktree and same build for both runs,colcon test --ctest-args -j 1 -R '^clang_tidy$':ROS2_MEDKIT_CLANG_TIDY_JOBSBoth runs report the same single pre-existing finding, so the change alters scheduling only, not what gets analysed.
To verify: configure with
-DENABLE_CLANG_TIDY=ONand check the registered command inbuild/<pkg>/CTestTestfile.cmake- it now ends in--jobs <n>.-DROS2_MEDKIT_CLANG_TIDY_JOBS=1restores the old behaviour, andros2_medkit_clang_tidy(JOBS <n>)overrides it for one package.The serialisation is already being worked around in the tree.
ament_clang_tidydefaults to a 300 s test timeout, andros2_medkit_gatewayregisters its check withTIMEOUT 3000- a tenfold raise.ros2_medkit_fault_managertakes the default and, measured on the same 16-core machine, its serial run exceeds it and the test fails by timeout; with the job count passed through it finishes in roughly a minute. So this is not only a wall-clock cost: on a machine with cores to spare, a package that never raised its timeout has a red gate for no reason other than scheduling.The
TIMEOUT 3000on the test is left as is - it was close to being reached by the serial run and is now comfortably clear.Checklist
No behaviour change for callers; the
ros2_medkit_cmakeREADME gained a section describing the module, the new knob, and why the two levels of parallelism must not both be enabled.