Skip to content

Use CCache to speed up build times in GitHub actions #5609

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

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

tautschnig
Copy link
Collaborator

We used to do this on Travis and CodeBuild, let's also port this to
GitHub actions.

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@tautschnig tautschnig requested a review from a team as a code owner November 17, 2020 11:40
@tautschnig tautschnig force-pushed the github-actions-ccache branch from c43a7b0 to 76dc705 Compare November 17, 2020 12:11
@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #5609 (1042403) into develop (f2d735a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5609   +/-   ##
========================================
  Coverage    69.32%   69.32%           
========================================
  Files         1241     1241           
  Lines       100443   100443           
========================================
  Hits         69636    69636           
  Misses       30807    30807           
Flag Coverage Δ
cproversmt2 43.08% <ø> (ø)
regression 66.22% <ø> (ø)
unit 32.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2d735a...1042403. Read the comment docs.

@tautschnig tautschnig force-pushed the github-actions-ccache branch 3 times, most recently from d6fafc9 to 73c8dd5 Compare November 17, 2020 14:47
@tautschnig tautschnig marked this pull request as draft November 17, 2020 16:26
@tautschnig tautschnig force-pushed the github-actions-ccache branch 12 times, most recently from e2d110b to fb5bb8c Compare November 18, 2020 17:38
@tautschnig tautschnig marked this pull request as ready for review November 18, 2020 17:39
@tautschnig
Copy link
Collaborator Author

FWIW, I am planning to do a separate PR to clean up the vs-2019-build-and-test configuration and, once done, also make it use clcache.

Copy link
Contributor

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

For me this looks good, but I'd be wary of merging something that's actively breaking CI, even if it promises to fix it later.

@NlightNFotis
Copy link
Contributor

Also, this won't be able to be merged because the check-vs-2019-build-and-test check is marked as required. Can the two PRs not be one?

@hannes-steffenhagen-diffblue
Copy link
Contributor

hannes-steffenhagen-diffblue commented Nov 19, 2020

Have you checked if this has a measurable performance improvement in subsequent builds?

EDIT NVM apparently it’s in the commit messages

@hannes-steffenhagen-diffblue
Copy link
Contributor

Looks like one java unit test is failing due to what looks like a pretty sketchy pretty function. Relies on a lot of platform specific behaviour which seems unnecessary.

We used to do this on Travis and CodeBuild, let's also port this to
GitHub actions. For any CMake builds, ccache will be used automatically
if available (as of d953327).

For Visual Studio, we'd need ccache/ccache#506
to be merged to use ccache.

On Ubuntu runners and OSX runners, the build time of a rebuild without
source code changes is down to under one minute (from 20 minutes).
ccache does not currently support msbuild, and we had successfully used
clcache on CodeBuild before. Build times (of a fully cached rebuild)
reduce from 34 minutes down to under 10 minutes.
@tautschnig tautschnig force-pushed the github-actions-ccache branch from fb5bb8c to 1042403 Compare November 19, 2020 13:57
@tautschnig
Copy link
Collaborator Author

@NlightNFotis

Also, this won't be able to be merged because the check-vs-2019-build-and-test check is marked as required. Can the two PRs not be one?

@hannes-steffenhagen-diffblue

Looks like one java unit test is failing due to what looks like a pretty sketchy pretty function. Relies on a lot of platform specific behaviour which seems unnecessary.

Argh, this was fixed in #5613. I should have rebased this earlier today, my apologies.

@thomasspriggs
Copy link
Contributor

The issue with the test actually appears to be a long-standing issue with the floating_point_to_java_string function when compiled under Visual Studio. I would say that the issue is with the function rather than the test. This is because 1) Java is usually intended to be platform independent and 2) because the doxygen for this function mentions not printing unnecessary zeros, but it is returning more zeros when compiled under visual studio than on other platforms. Therefore I would prefer to see the function under test fixed, rather than in increasing number of exceptions for different compilers added to the unit test.

@tautschnig
Copy link
Collaborator Author

The issue with the test actually appears to be a long-standing issue with the floating_point_to_java_string function when compiled under Visual Studio. I would say that the issue is with the function rather than the test. This is because 1) Java is usually intended to be platform independent and 2) because the doxygen for this function mentions not printing unnecessary zeros, but it is returning more zeros when compiled under visual studio than on other platforms. Therefore I would prefer to see the function under test fixed, rather than in increasing number of exceptions for different compilers added to the unit test.

+100, but #5613 was the quick way to stop the bleeding.

@tautschnig tautschnig merged commit c907b2b into diffblue:develop Nov 19, 2020
@tautschnig tautschnig deleted the github-actions-ccache branch November 19, 2020 15:13
@thomasspriggs
Copy link
Contributor

It is worth noting that the total cache size for the whole repository is limited to 5GB. Source - https://github.com/actions/cache#cache-limits

@tautschnig
Copy link
Collaborator Author

It is worth noting that the total cache size for the whole repository is limited to 5GB. Source - https://github.com/actions/cache#cache-limits

Yes, that's why I limited each job to 500 MB, I should perhaps have noted that in the commit message. GitHub also compresses the tar ball, making it around 50 MB it seems, so we can have quite a lot of cache keys before old ones are expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants