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

[GitHub] Remove redundant cache key prefix #76914

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

linux4life798
Copy link
Contributor

Remove the redundant sccache cache key prefix.
This prefix is already added by the ccache action, which results in cache keys like "sccache-sccache-ubuntu-...".

See the following source lines as proof:
https://github.com/hendrikmuhs/ccache-action/blob/2a51777f6f64b7b7bea213601acba8f5f4fdbe03/src/restore.ts#L22-L23

Remove the redundant sccache cache key prefix.
This prefix is already added by the ccache action,
which results in cache keys like "sccache-sccache-ubuntu-...".

See the following source lines as proof:
https://github.com/hendrikmuhs/ccache-action/blob/2a51777f6f64b7b7bea213601acba8f5f4fdbe03/src/restore.ts#L22-L23
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-github-workflow

Author: Craig Hesling (linux4life798)

Changes

Remove the redundant sccache cache key prefix.
This prefix is already added by the ccache action, which results in cache keys like "sccache-sccache-ubuntu-...".

See the following source lines as proof:
https://github.com/hendrikmuhs/ccache-action/blob/2a51777f6f64b7b7bea213601acba8f5f4fdbe03/src/restore.ts#L22-L23


Full diff: https://github.com/llvm/llvm-project/pull/76914.diff

1 Files Affected:

  • (modified) .github/workflows/llvm-project-tests.yml (+1-1)
diff --git a/.github/workflows/llvm-project-tests.yml b/.github/workflows/llvm-project-tests.yml
index 02b1ab75e960ec..fadaea129e5088 100644
--- a/.github/workflows/llvm-project-tests.yml
+++ b/.github/workflows/llvm-project-tests.yml
@@ -87,7 +87,7 @@ jobs:
           # enough cache space for all the tests to run at once and still
           # fit under the 10 GB limit.
           max-size: 500M
-          key: sccache-${{ matrix.os }}
+          key: ${{ matrix.os }}
           variant: sccache
       - name: Build and Test
         uses: llvm/actions/build-test-llvm-project@main

Copy link
Collaborator

@tru tru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think this might cause some of the new runs to start cold, but given this only runs on the release branch, that shouldn't be an issue anyways as the 17.x release cycle is over.

@tru
Copy link
Collaborator

tru commented Jan 4, 2024

On a side note, I think we should look into how many cache hits we get in a build and see if we can improve it, I don't think we have good enough hits since the builds still takes quite a while. In our internal jobs we run ccache -s after each build to see how many hits and misses we have, maybe we can do something similar with sscache? Just writing it here to see if someone wants to pick that task up :)

@tstellar
Copy link
Collaborator

tstellar commented Jan 4, 2024

@tru The jobs already print the ccache stats when they are done. Its the 'Post Setup ccache' step.

@tru
Copy link
Collaborator

tru commented Jan 4, 2024

@tru The jobs already print the ccache stats when they are done. Its the 'Post Setup ccache' step.

Oh great, I must have missed it under the separate step. I will look at some of them.

@linux4life798
Copy link
Contributor Author

linux4life798 commented Jan 5, 2024

On a side note, I think we should look into how many cache hits we get in a build and see if we can improve it, I don't think we have good enough hits since the builds still takes quite a while. In our internal jobs we run ccache -s after each build to see how many hits and misses we have, maybe we can do something similar with sscache? Just writing it here to see if someone wants to pick that task up :)

Yeah, I've been doing a bit of active research on this. It seems to me that we need to ensure that their are periodic builds on main (the default branch), since caches will be restored from the default branch (main) cache if none exist in the branch being built (might be a PR branch). Additionally, we could consider disable cache saving on non-main branches (like pull requests) to ensure that the main branch cache doesn't get evicted and can continue to grow towards that 10GB cache quota. We can configure sccache in the action to keep its own cache below a given size. I saw on the Discord that there were a lot of opinions and options, but I think we could probably just get this to work reasonably well within GitHub.

If it wasn't already known, a warm cache can bring a build time down from roughly 1.33 hours to 9 mins (building libclang on the ubuntu runner).

@tstellar tstellar merged commit cf02e6e into llvm:main Jan 6, 2024
5 checks passed
@linux4life798 linux4life798 deleted the remove-extra-sccache-tag-prefix branch January 6, 2024 06:15
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
Remove the redundant sccache cache key prefix.
This prefix is already added by the ccache action, which results in
cache keys like "sccache-sccache-ubuntu-...".

See the following source lines as proof:

https://github.com/hendrikmuhs/ccache-action/blob/2a51777f6f64b7b7bea213601acba8f5f4fdbe03/src/restore.ts#L22-L23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants