ci: A few fixes of `ccache` issues #27084

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:230211-cccc changing 5 files +7 −2

hebasto commented at 11:51 AM on February 11, 2023: member
Combined #27077 and #27083 for the sake of more representative testing.

The second run looks promising:
- "MSan" task -- 98.74 %
- "ASan + LSan + UBSan + integer" task -- 100.0 %
- "multiprocess" task -- 99.35 %
- "macOS 13 native" task -- 100.0%
- "Android APK" task -- 100.0%
Although, the "previous releases" task has a 66.10 % cache hit rate. It needs more investigation (not in this PR).
DrahtBot commented at 11:51 AM on February 11, 2023: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #27360 (ci: use LLVM/clang-16 in native_asan job by fanquake)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label Tests on Feb 11, 2023

in .cirrus.yml:8 in b49fdb1984 outdated

   4 | @@ -5,8 +5,9 @@ env:  # Global defaults
   5 |    TEST_RUNNER_PORT_MIN: "14000"  # Must be larger than 12321, which is used for the http cache. See https://cirrus-ci.org/guide/writing-tasks/#http-cache
   6 |    CI_FAILFAST_TEST_LEAVE_DANGLING: "1"  # Cirrus CI does not care about dangling process and setting this variable avoids killing the CI script itself on error
   7 |    CCACHE_SIZE: "200M"
   8 | -  CCACHE_DIR: "/tmp/ccache_dir"
   9 | +  CCACHE_DIR: '${CIRRUS_WORKING_DIR}/ccache_dir'

maflcko commented at 12:11 PM on February 11, 2023:

This will still be in the same volume (tmp), so why is this changed?

hebasto commented at 12:13 PM on February 11, 2023:

I think the difference for Cirrus CI is not using absolute path /tmp/....

maflcko commented at 4:51 PM on February 13, 2023:

I am still not sure what this means and which paths are preferred or discouraged

hebasto commented at 8:32 AM on February 21, 2023:

#27084 (comment)

hebasto marked this as ready for review on Feb 11, 2023
hebasto commented at 3:29 PM on February 11, 2023: member

The PR description has been updated with some statistics.
fanquake commented at 5:02 PM on February 20, 2023: member

Most of the changes here look straight forward enough, and clearly improve ccache usage, but this is blocked on at least an answer to this question: #27084 (review).
hebasto commented at 8:31 AM on February 21, 2023: member

@fanquake @MarcoFalke

... but this is blocked on at least an answer to this question: #27084 (comment).

~I've dropped the first commit and faced some weird issues which I saw in #27083 before. Going to provide steps to reproduce that unwanted behavior.~

Making this PR a draft for now.
hebasto marked this as a draft on Feb 21, 2023

ci: Distinguish `ccache` cache by tasks

This change avoids any possible cache binary incompatibility.

0df55c5bc9

ci: Make `ccache` hashing content of compiler binary

By default, `ccache` includes the modification time (`mtime`) and size
of the compiler in the hash to ensure that results retrieved from the
cache are accurate. But in CI environment compiler's `mtime` can be
unique each run.

3b7625289f

ci: Adjust `CCACHE_SIZE` for some tasks

This change improves cache hit rates for the touched CI tasks.

9a5c00cb1a

hebasto marked this as ready for review on Feb 21, 2023
hebasto force-pushed on Feb 21, 2023
hebasto commented at 12:59 PM on February 21, 2023: member

The questionable commit has been dropped.

Ready to re-review.
in .cirrus.yml:39 in 0df55c5bc9 outdated
```
  35 | @@ -36,6 +36,8 @@ main_template: &MAIN_TEMPLATE
  36 |    timeout_in: 120m  # https://cirrus-ci.org/faq/#instance-timed-out
  37 |    ccache_cache:
  38 |      folder: "/tmp/ccache_dir"
  39 | +    fingerprint_script: echo ${CIRRUS_TASK_NAME}
```
maflcko commented at 3:01 PM on February 21, 2023:

Isn't this already the default?

By default the task name is used as a fingerprint value.

https://cirrus-ci.org/guide/writing-tasks/#cache-instruction

fanquake commented at 2:51 PM on February 27, 2023:

Isn't this already the default?

Yep looks like it.

in .cirrus.yml:40 in 0df55c5bc9 outdated

  35 | @@ -36,6 +36,8 @@ main_template: &MAIN_TEMPLATE
  36 |    timeout_in: 120m  # https://cirrus-ci.org/faq/#instance-timed-out
  37 |    ccache_cache:
  38 |      folder: "/tmp/ccache_dir"
  39 | +    fingerprint_script: echo ${CIRRUS_TASK_NAME}
  40 | +    reupload_on_changes: true

maflcko commented at 3:02 PM on February 21, 2023:

Isn't this already the default?

true otherwise

https://cirrus-ci.org/guide/writing-tasks/#cache-instruction

maflcko approved
maflcko commented at 3:04 PM on February 21, 2023: member

lgtm
hebasto marked this as a draft on Feb 27, 2023
hebasto commented at 9:16 AM on March 1, 2023: member
Well, speaking of the removed questionable commit, I've finally managed to provide steps to reproduce unexpected behavior without that commit:
1. Complete all tasks.
2. Re-run, for example, the "32-bit + dash [gui] [CentOS 8]" task. As expected, ccache achieves 100% hit.
3. Add a commit which does not change any code and run CI job. It is natural to expect that ccache in the "32-bit + dash [gui] [CentOS 8]" task will 100% hit again. But that is not the case. Somehow, ccache's cache is "contaminated", which, in turn, causes some misses.
Besides the description above, I have no decent explanation of what is really happening. But s/tmp/ccache_dir/${CIRRUS_WORKING_DIR}/ccache_dir/ solves this issue.
maflcko commented at 9:43 AM on March 1, 2023: member

It would be good to double check if this is recommended "officially" by Cirrus CI. In any case CIRRUS_WORKING_DIR isn't under /tmp/ on macos, so maybe it makes sense for that reason already?
maflcko commented at 9:44 AM on March 1, 2023: member

I guess instead of ${CIRRUS_WORKING_DIR}/ccache_dir you can also just write ccache_dir, as CIRRUS_WORKING_DIR is implicit?
hebasto commented at 11:51 AM on March 1, 2023: member

I guess instead of ${CIRRUS_WORKING_DIR}/ccache_dir you can also just write ccache_dir, as CIRRUS_WORKING_DIR is implicit?

While it works for the folder property of the ccache_cache, it seems the CCACHE_DIR variable is supposed to contain an absolute path.
maflcko commented at 4:21 PM on March 13, 2023: member

lgtm. While it would be good to be able to point to docs, I don't think this is required. Happy to review if you move this out of draft.
DrahtBot added the label Needs rebase on May 2, 2023
DrahtBot commented at 1:44 PM on May 2, 2023: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
fanquake commented at 12:05 PM on May 16, 2023: member

What is the status of this
hebasto commented at 2:42 PM on May 17, 2023: member

What is the status of this

I've analyzed the recent ccache summaries at https://github.com/bitcoin/bitcoin/commit/a75c77ea903c100531e0fc5fde94bb9b52642145.

I don't think this PR can improve them.
hebasto closed this on May 17, 2023
bitcoin locked this on May 16, 2024

Contributors

Labels

Linked (view graph)

#27077 ci: Make `ccache` hashing content of compiler binary #27083 ci: Improve `ccache` cache hit rates

ci: A few fixes of `ccache` issues #27084

Reviews

Conflicts