Skip to content

Conversation

janisozaur
Copy link
Contributor

By default action is set up to:

  1. Use 500MB size for cache
  2. Compress the cache
  3. Show the cache stats post-build to ensure it's used efficiently

By default action is set up to:
1. Use 500MB size for cache
2. Compress the cache
3. Show the cache stats post-build to ensure it's used efficiently
@janisozaur
Copy link
Contributor Author

All the jobs report 0 hits, this needs a bit more inspection

@janisozaur janisozaur marked this pull request as draft February 9, 2021 14:06
Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

Congratulations for making PR 1000, a milestone for Flang. Hope you will be able to complete this PR.

BTW, is this hobby work?

- uses: actions/checkout@v2

- name: ccache
uses: hendrikmuhs/ccache-action@v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use this custom cacching action over the Github-provided one (https://github.com/actions/cache)?

Copy link
Contributor Author

@janisozaur janisozaur Feb 9, 2021

Choose a reason for hiding this comment

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

Yes - the GitHub cache action only provides a way of caching, the use of which you actually have to implement yourself. The ccache action used here provides that and builds on top of that - installs ccache itself, configures some options (the ones I mentioned in the opening comment), ensures ccache is available in the $PATH, creates a log entry with stats.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only concern is using an action in a private individual's repo.

WDYT, @bryanpkc @shivaramaarao

Copy link
Collaborator

Choose a reason for hiding this comment

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

The TypeScript code looks pretty straightforward. Does the presence of the action in the GitHub Marketplace ensure its continued availability? Will it disappear suddenly and break our CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt presence on the actions marketplace guarantees anything. If that's a concern for you, you may consider forking the project into flang-compiler organisation. Personally though I wouldn't recommend that, this PR is quite small and self-contained. Should the action ever be removed, fallback to previous version is easy enough.

Alternatively, ccache installation and caching can be rewritten in bash so that you don't need to rely on external actions, but I'm not keen on doing that when current solution seems sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@janisozaur I agree with your view. I don't have a concern with this use.

@janisozaur
Copy link
Contributor Author

BTW, is this hobby work?

Yes, I just happen to know @michalpasztamobica

Previously I haven't noticed I was editing only one of the jobs in the matrix. It should be good now.

@janisozaur janisozaur marked this pull request as ready for review February 9, 2021 14:22
@janisozaur
Copy link
Contributor Author

Caches now populate correctly and in the case of GCC 9, it even got used already:

cache directory                     /home/runner/work/flang/flang/.ccache
primary config                      /home/runner/.ccache/ccache.conf
secondary config      (readonly)    /etc/ccache.conf
stats updated                       Tue Feb  9 14:19:15 2021
cache hit (direct)                     0
cache hit (preprocessed)              12
cache miss                          1406
cache hit rate                      0.85 %
cleanups performed                     0
files in cache                      2821
cache size                          17.7 MB
max cache size                     500.0 MB

As it was only for a minor part of the build, you can see it in the line:

cache hit (preprocessed)              12

Each job will have its own cache.
This will keep size of individual cache lower and increase hit rate by
not trying to overwrite the same cache over and over again.
@janisozaur
Copy link
Contributor Author

ping

@bryanpkc
Copy link
Collaborator

@janisozaur Would you consider creating a similar PR to implement ccache for https://github.com/flang-compiler/classic-flang-llvm-project? IMO the LLVM build will benefit much more from caching than the Flang build, since it takes way longer.

@janisozaur
Copy link
Contributor Author

Sure!

@janisozaur
Copy link
Contributor Author

Just had a look at that repo - it seems so much larger than this one, it may exceed the (default) cache capacity available. GitHub offers up to 5GB (https://github.com/actions/cache#cache-limits) so it should be possible to bump it a bit higher. This may need some manual fiddling with options.

@janisozaur
Copy link
Contributor Author

flang-compiler/classic-flang-llvm-project#17

Copy link
Collaborator

@bryanpkc bryanpkc left a comment

Choose a reason for hiding this comment

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

LGTM.

@kiranchandramohan kiranchandramohan merged commit beec8ee into flang-compiler:master Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants