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

[GCB] Speed up clang-tidy #6170

Closed
devjgm opened this issue Apr 1, 2021 · 2 comments · Fixed by #6228
Closed

[GCB] Speed up clang-tidy #6170

devjgm opened this issue Apr 1, 2021 · 2 comments · Fixed by #6228
Labels
type: cleanup An internal cleanup or hygiene concern.

Comments

@devjgm
Copy link
Contributor

devjgm commented Apr 1, 2021

Our clang-tidy build currently uses cmake and it takes about 20 min to do a complete build (on 32 cores), even when no files changed. We need this to be faster for our PR builds.

In our kokoro builds we use a hack in our PR builds where we only run clang-tidy on changed files:

if [[ "${CLANG_TIDY:-}" == "yes" && ("${KOKORO_JOB_TYPE:-}" == "PRESUBMIT_GITHUB" || "${KOKORO_JOB_TYPE:-}" == "PRESUBMIT_GIT_ON_BORG") ]]; then
# For presubmit builds we only run clang-tidy on the files that have changed
# w.r.t. the target branch.
TARGET_BRANCH="${KOKORO_GITHUB_PULL_REQUEST_TARGET_BRANCH:-${BRANCH}}"
io::log_yellow "Build clang-tidy prerequisites"
"${CMAKE_DRIVER}" ${CMAKE_DRIVER_ARGS[@]+"${CMAKE_DRIVER_ARGS[@]}"} cmake \
--build "${BINARY_DIR}" --target google-cloud-cpp-protos
io::log_yellow "Run clang-tidy on changed files in a presubmit build"
HEADER_FILTER_REGEX=$(clang-tidy -dump-config |
sed -n "s/HeaderFilterRegex: *'\([^']*\)'/\1/p")
SOURCE_FILTER_REGEX='google/cloud/.*\.cc$'
# Get the list of modified files.
readonly MODIFIED=$(git diff --diff-filter=d --name-only "${TARGET_BRANCH}")
# Run clang_tidy against files that regex match the first argument (less some
# exclusions). Any remaining arguments are passed to clang-tidy.
run_clang_tidy() {
local -r file_regex="$1"
shift
grep -E "${file_regex}" <<<"${MODIFIED}" |
grep -v generator/integration_tests/golden |
xargs --verbose -d '\n' -r -n 1 -P "${NCPU}" clang-tidy \
-p="${BINARY_DIR}" "$@"
}
# We disable the following checks:
# misc-unused-using-decls
# readability-redundant-declaration
# because they produce false positives when run on headers.
# For more details, see issue #4230.
run_clang_tidy "${HEADER_FILTER_REGEX}" \
--checks="-misc-unused-using-decls,-readability-redundant-declaration"
# We don't need to exclude any checks for source files.
run_clang_tidy "${SOURCE_FILTER_REGEX}"
fi

We cannot do this (easily) with GCB because GCB builds do not include the .git directory so we cannot tell which files changed. I mean, we could fetch the PR and figure it out, so that is an option.

Some options, in rough order of preference:

  1. Switch to running clang-tidy with bazel, which is better about caching results and should only run clang-tidy on changed TUs. However, there's no out-of-the-box solution for this. I've not tested this, but https://github.com/erenon/bazel_clang_tidy looks promising.
  2. Continue using cmake + clang-tidy, but write a clang-tidy wrapper that attempts to cache the results. Very roughly I imagine the wrapping being invoked w/ the full clang-tidy command line, which includes all the compile commands, then the wrapper adds -E to dump the pre-processed output, and that gets hashed along with the contents of our .clang-tidy file, and the result cached. On a cache miss, run-clang-tidy. On a cache hit, return the cached result. It's possible that we could use ccache to do the caching for us. Note: this idea is very experimental and I have no idea if it's even possible
  3. Try to create a working .git directory in our GCB build environment so that we can use git commands to determine which files changed, then use the same hack that I mentioned above that we use with Kokoro.
@devjgm devjgm added the type: cleanup An internal cleanup or hygiene concern. label Apr 1, 2021
@devjgm
Copy link
Contributor Author

devjgm commented Apr 12, 2021

My first stab at getting https://github.com/erenon/bazel_clang_tidy to work failed due to erenon/bazel_clang_tidy#2

@devjgm
Copy link
Contributor Author

devjgm commented Apr 12, 2021

I just found https://github.com/matus-chochlik/ctcache, which appears to be a new attempt at a clang-tidy cache for use with CMake, and it seems pretty nice. It's working (I think) in my first quick experiments.

devjgm added a commit to devjgm/google-cloud-cpp that referenced this issue Apr 12, 2021
Fixes googleapis#6170

This PR starts using https://github.com/matus-chochlik/ctcache, which is
a clang-tidy cache, to speed up our clang-tidy builds. This PR also
re-enables clang-tidy for PR builds because they should be fast enough
now.

In my experiments using ctcache vs using no clang-tidy cache, I see
builds taking ~7% *longer* on clean full builds. This makes sense as the
cache needs to compute hashes, check for cache hits, etc. But on builds
with a populated `~/.cache/ctcache` I see builds completing ~50%
*faster*. I think these stats look good, and I think this is worth
trying.
Move Linux builds to Cloud Build automation moved this from To Do to Done Apr 12, 2021
devjgm added a commit that referenced this issue Apr 12, 2021
Fixes #6170

This PR starts using https://github.com/matus-chochlik/ctcache, which is
a clang-tidy cache, to speed up our clang-tidy builds. This PR also
re-enables clang-tidy for PR builds because they should be fast enough
now.

In my experiments using ctcache vs using no clang-tidy cache, I see
builds taking ~7% *longer* on clean full builds. This makes sense as the
cache needs to compute hashes, check for cache hits, etc. But on builds
with a populated `~/.cache/ctcache` I see builds completing ~50%
*faster*. I think these stats look good, and I think this is worth
trying.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup An internal cleanup or hygiene concern.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

1 participant