build: Enhance Ccache performance across worktrees and build trees #30861
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30861. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
I tested this, and got about 90%: ccache -C
ccache --zero-stats
cmake -B some_build_dir -DWITH_CCACHE=ON
cmake --build some_build_dir -j17
ccache --show-stats
Cacheable calls: 445 / 445 (100.0%)
Hits: 0 / 445 ( 0.00%)
Direct: 0
Preprocessed: 0
Misses: 445 / 445 (100.0%)
Local storage:
Cache size (GiB): 0.2 / 30.0 ( 0.75%)
Hits: 0 / 445 ( 0.00%)
Misses: 445 / 445 (100.0%)
ccache --zero-stats
cmake -B /tmp/some_other_build_dir -DWITH_CCACHE=ON
cmake --build /tmp/some_other_build_dir -j17
ccache --show-stats
Cacheable calls: 445 / 445 (100.0%)
Hits: 394 / 445 (88.54%)
Direct: 394 / 394 (100.0%)
Preprocessed: 0 / 394 ( 0.00%)
Misses: 51 / 445 (11.46%)
Local storage:
Cache size (GiB): 0.2 / 30.0 ( 0.76%)
Hits: 394 / 445 (88.54%)
Misses: 51 / 445 (11.46%) |
cmake/ccache.cmake
Outdated
| list(APPEND CMAKE_CXX_COMPILER_LAUNCHER ${CCACHE_EXECUTABLE}) | ||
| foreach(lang IN ITEMS C CXX OBJCXX) | ||
| set(CMAKE_${lang}_COMPILER_LAUNCHER | ||
| ${CCACHE_EXECUTABLE} base_dir=${CMAKE_BINARY_DIR} |
There was a problem hiding this comment.
In commit "build: Improve ccache performance for different build directories" (66b73bb)
Reading https://ccache.dev/manual/latest.html#_configuration_options it seems like it would make sense in most cases to set base_dir to CMAKE_SOURCE_DIR not CMAKE_BINARY_DIR.
For example if CMAKE_SOURCE_DIR is /home/bitcoin and CMAKE_BINARY_DIR is /home/bitcoin/build, it sounds like ccmake will not rewrite the include path -I/home/bitcoin/src to -I../src for greater cache reuse if base_dir=CMAKE_BINARY_DIR is used, because /home/bitcoin/src is not a subdirectory of /home/bitcoin/build
But it would rewrite the include path if CMAKE_SOURCE_DIR were used, because /home/bitcoin/src is a subdirectory of /home/bitcoin.
There was a problem hiding this comment.
CCACHE_BASEDIR=${CMAKE_BINARY_DIR} is effective when building in different build directories from the same source directory.
CCACHE_BASEDIR=${CMAKE_SOURCE_DIR} works for building from different source directories.
We need to choose one of these two options.
There was a problem hiding this comment.
CCACHE_BASEDIR=${CMAKE_BINARY_DIR}is effective when building in different build directories from the same source directory.
CCACHE_BASEDIR=${CMAKE_SOURCE_DIR}works for building from different source directories.We need to choose one of these two options.
Thanks, that clarifies a lot. I only do the second not the first, because I use git worktrees, and I assumed this PR was trying to make git worktrees and multiple checkouts work better. Maybe people who work on build systems are likely to have many different build directories inside the same source directory, but that would seem like an unusual thing for most other developers. Especially to have multiple build directories building with the same compilation options, because if compile options are different ccache command lines wouldn't match anyway.
Is there a real use-case for having multiple different build directories with the same compilation options in the same source directory? And optimizing to have cache hits in that case?
Also maybe I need to think about it more, but I'm not sure why choosing CMAKE_BINARY_DIR would be better than CMAKE_SOURCE_DIR in either case as long a CMAKE_BINARY_DIR is a subdirectory of CMAKE_SOURCE_DIR. Are we trying to optimize for cases where CMAKE_BINARY_DIR is not a subdirectory?
There was a problem hiding this comment.
Also maybe I need to think about it more, but I'm not sure why choosing CMAKE_BINARY_DIR would be better than CMAKE_SOURCE_DIR in either case as long a CMAKE_BINARY_DIR is a subdirectory of CMAKE_SOURCE_DIR. Are we trying to optimize for cases where CMAKE_BINARY_DIR is not a subdirectory?
I don't think it is related to whether CMAKE_BINARY_DIR is a subdirectory of CMAKE_SOURCE_DIR or not.
Due to
Line 9 in 07c7c96
all objects are compiled with the
-I/absolute/path/to/build/dir and -I/absolute/path/to/source/dir flags, and any difference in either will trigger a cache miss.
The "Cache compilations with ccache" section of the Productivity Notes still applicable for this PR branch except for the last commit, which was mostly motivated by #30811 (comment).
There was a problem hiding this comment.
I don't think it is related to whether
CMAKE_BINARY_DIRis a subdirectory ofCMAKE_SOURCE_DIRor not.
Ccache documentation describes behavior of base_dir: "ccache will rewrite absolute paths into paths relative to the current working directory, but only absolute paths that begin with base_dir"
So if CMAKE_BINARY_DIR is a subdirectory of CMAKE_SOURCE_DIR, and base_dir is specified as CMAKE_SOURCE_DIR, then that should be a strict improvement because it will allow cache hits across multiple source directories, and multiple build directories, as long as build directories are not outside the source directories.
There was a problem hiding this comment.
last commit, which was mostly motivated by #30811 (comment).
Sorry, I was just testing, not trying to imply that this is the common case. Using worktrees (different source dirs) seems more common than different build dirs for the same cmake config.
There was a problem hiding this comment.
So if CMAKE_BINARY_DIR is a subdirectory of CMAKE_SOURCE_DIR, and base_dir is specified as CMAKE_SOURCE_DIR, then that should be a strict improvement because it will allow cache hits across multiple source directories, and multiple build directories, as long as build directories are not outside the source directories.
For the latter case, using different build directories will result in cache misses anyway, right?
Anyway, I agree with @maflcko and @ryanofsky that using git worktrees should be prioritised.
There was a problem hiding this comment.
So if CMAKE_BINARY_DIR is a subdirectory of CMAKE_SOURCE_DIR, and base_dir is specified as CMAKE_SOURCE_DIR, then that should be a strict improvement because it will allow cache hits across multiple source directories, and multiple build directories, as long as build directories are not outside the source directories.
For the latter case, using different build directories will result in cache misses anyway, right?
It seems like that should be true but I haven't experimented or thought about this much and am just going off of the documentation. If cmake adds include paths inside the build directory, maybe for generated headers, or for headers are copied or linked to from there, I could see imagine CMAKE_BINARY_DIR being better to use than CMAKE_SOURCE_DIR in some cases. But naively I would expect CMAKE_SOURCE_DIR being better in most cases, so that led me to ask the question.
There was a problem hiding this comment.
I only do the second not the first, because I use git worktrees...
Thanks for your feedback. Your concerns should be addressed in the recent push.
There was a problem hiding this comment.
Reworked to work equally well across both Git's wortktrees and build trees.
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
cmake/ccache.cmake
Outdated
| list(APPEND CMAKE_CXX_COMPILER_LAUNCHER ${CCACHE_EXECUTABLE}) | ||
| foreach(lang IN ITEMS C CXX OBJCXX) | ||
| set(CMAKE_${lang}_COMPILER_LAUNCHER | ||
| ${CCACHE_EXECUTABLE} base_dir=${CMAKE_BINARY_DIR} |
There was a problem hiding this comment.
In commit "build: Improve ccache performance for different build directories" (66b73bb)
Now that base_dir is set by the build system, would it be fine to remove this from the prod notes?
$ git grep --line-number base_dir doc/productivity.md
doc/productivity.md:42:base_dir = /home/yourname # or wherever you keep your source files
doc/productivity.md:45:Note: base_dir is required for ccache to share cached compiles of the same file across different repositories / paths; it will only do this for paths under base_dir. So this option is required for effective use of ccache with git worktrees (described below).
doc/productivity.md:47:You _must not_ set base_dir to "/", or anywhere that contains system headers (according to the ccache docs).
There was a problem hiding this comment.
In the recent push, this branch relies on setting the base_dir option by the user because the required value depends on actual user's build environment.
On Ubuntu 24.04, I've got: for the build in the second directory. @fanquake What is your system and Ccache version? |
That was on a macOS machine, with 4.10.2. Retested just now, and got the same: |
|
Just tested the same on a Fedora box (ccache 4.10.2) and ended up with |
What if you On Fedora, |
|
Needs rebase and CI failures fixed (they are true ones) |
In that case on Fedora it is |
66b73bb to
0a7e4e8
Compare
7dd1442 to
9ea2db2
Compare
If this is expected on systems where |
9ea2db2 to
3f8afce
Compare
|
The PR has been reworked to keep compatibility with git worktrees. See this discussion. The PR description has been updated accordingly. |
|
Seems like you missed some review comments? Seems like #30861 (comment) is now also more relevant given #30905, so it'd be good to know how this is going to be handed going forward. |
To enhance the user experience when using ccache, we utilize the CMake's `<LANG>_COMPILER_LAUNCHER` target properties, which are only supported by the Makefile and Ninja generators.
To maximize performance, it is essential to follow the recommendations in the Productivity Notes: set the `base_dir` ccache configuration option or use the `CCACHE_BASEDIR` environment variable.
aeb3977 to
82a9f24
Compare
|
Thanks to all for the review! Your feedback has been addressed. Additionally, #31771 has been fixed. |
| list(APPEND configure_warnings | ||
| "CMAKE_${lang}_COMPILER_LAUNCHER is already set. Skipping built-in ccache support for ${lang}." | ||
| ) |
There was a problem hiding this comment.
Is making this a configure_warning compatible with #31665? Maybe it would be a benefit if CI complained when unintentionally skipping ccache?
There was a problem hiding this comment.
Would you mind elaborating on your suggestion?
There was a problem hiding this comment.
Maybe better expressed as: Is trying to both override the compiler launcher and make configure_warnings errors a use case that should be supported? If so, making this message a configure_warning is incompatible with #31665 since that makes configure_warnings fatal errors if WCONFIGURE_ERROR=ON.
If it is not a use case that should be supported, then making this message a configure_warning might be good together with #31665, since it would cause an error in CI if ccache ever got disabled because a compiler launcher was set, but maybe there isn't a realistic situation where something like that could happen unintentionally.
There was a problem hiding this comment.
It's also fair if you think #31665 is a bad approach and this is more evidence of that.
There was a problem hiding this comment.
Code review ACK 82a9f24 since this seems like a reasonable change, but I'm confused about why it actually doing anything and agree with stickies concern #30861 (review) that forcing CCACHE_NOHASHDIR=1 might not be ideal, especially because according to documentation for hash_dir it sounds like if CWD is not hashed and -fdebug-prefix-map is not used, objects could be built with the wrong debug information.
But mainly I'm just confused why this change has any effect at all given that hash_dir documentation says "The CWD will not be included in the hash if base_dir is set (and matches the CWD) and the compiler option -fdebug-prefix-map is used." It seems like we are doing (or requiring) both of these things so I don't see why the hash_dir setting should change anything. The results posted #30861 (comment) do seem to show it having an effect though, so think I must be misunderstanding something.
Also, I'm confused by why we need to require manually setting base_dir instead of setting it automatically to CMAKE_SOURCE_DIR. I don't understand comment
#30861 (comment) about why this would fail to cache multiple build directories, since most of the time build directories will be under the source directory.
| if(NOT MSVC) | ||
| # The <LANG>_COMPILER_LAUNCHER target property, used to integrate | ||
| # Ccache, is supported only by the Makefiles and Ninja generators. | ||
| if(CMAKE_GENERATOR MATCHES "Make|Ninja") |
There was a problem hiding this comment.
re: #30861 (comment)
In commit "cmake: Clarify ccache support for different generators" (c19a187)
Thanks! Reworked per your feedback.
It is hard to tell from reading this thread if the "I think we could drop this check entirely" suggestion and other suggestions have been addressed or not. The check appears to be unchanged. Would be good to clear this up.
Setting |
I don't understand why it wouldn't be helpful when a worktree is outside the repository root. If source directories are I do understand that setting Separately, I'm still confused about why setting hash_dir has an effect (mentioned #30861 (review)) so there are definitely things I'm not grasping here, but in any case this change seems at least safe and I am happy if it works in practice. |
Right. As for the PR author, it was not clear to me which cases are more common and which are less common, so I tried to take a more general approach. Ultimately, the mentioning of |
I'm wondering in which scenarios a user might wish to disable the
For the CWD to be omitted from hashing when a |
|
Looks like this missed feature-freeze and can be removed from the milestone for now? |
|
I'm going to re-add it, it'd be good to fix because there are a number of regressions here. |
|
There is also a risk of adding even more regressions due to Edit: Unrelated issue split up to #31957 |
|
Closing and marking "Up for grabs". |
|
re: #30861 (comment)
@maflcko could you clarify this? Are you talking about the problem where objects might be built with the wrong paths in debug info mentioned #30861 (review) or a different problem? The debug path problem didn't seem like a real risk as long as we are specifying re: #30861 (comment)
I don't think there are any. Just mentioned this because
Right, the documentation I quoted says the base_dir must match the CWD. But that just means the CWD needs "begin with base_dir" as stated in the base_dir documentation. So it seems like regardless of how hash_dir is set, CWD should not be included in the hash anyway, so it is unclear why this PR has any effect. The change seems good and does seem to be helping according to #30861 (comment) but I don't understand how it actually works if the documentation is accurate. |
Without `hash_dir=false`, ccache will not be shared across worktrees and buildtrees, since ccache will include the current working directory in hashes. See bitcoin#30861 and https://ccache.dev/manual/4.10.html#config_hash_dir
Without `hash_dir=false`, ccache will not be shared across worktrees and buildtrees, since ccache will include the current working directory in hashes. See bitcoin#30861 and https://ccache.dev/manual/4.10.html#config_hash_dir
Specifically, which condition are you referring to? |
|
To maximize performance, it is essential to follow the recommendations in the Productivity Notes: set the
base_dirCcache configuration option or use theCCACHE_BASEDIRenvironment variable:Here are examples of usage on Ubuntu 24.04:
This PR addresses this comment:
Fixes #31771.
Form https://github.com/bitcoin/bitcoin/actions/runs/13215680798/job/36894673006: