Add ccache support to Dockerfiles and build workflow#892
Add ccache support to Dockerfiles and build workflow#892lukovdm wants to merge 28 commits intostormchecker:masterfrom
Conversation
|
It seems like using ccache together with precompiled headers (PCH) does not really work. You get a really low ccache hit rate like this: I have done a quick benchmark (on a 24 core machine) of turning PCH on and off with and without a warm cache. These are the results:
So with a cold cache disabling PCH costs time. But with a warm cache all compile calls actually hit the cache, and it is thus much faster. Most time was taken up by the dependencies. For the GitHub actions CI we would always have a warmish cache, and thus it seems like it would be worth it to me to disable PCH for them. For people building storm themselves I don't know, it depends on how much we value cold cache builds versus warm cache builds. |
…kflow; add benchmark script for compile time analysis
|
Nice idea @lukovdm! I think it is good to try to decrease the building times in the CI. Some thoughts:
|
|
Thanks for the questions.
Also, some GitHub workflows are not properly caching yet, but the once that are, are running in 4-10 minutes. |
|
Thanks for the detailed answers. One thing I noticed looking at the latest CI run, for example here is that carl and sylvan seem to be rebuild each time. I think that might be due to the way how we configure the fetchcontent. |
|
Thanks Luko! Do you have any profiling of the compilation process with a warm start? We do have some ways of profiling storm compilation processes and it would be nice to know where the time is lost. I second Matthias: For the main branch, PCH seems preferable, in particular as I care about the cold start compile time a lot. (Although this is shifting with more people using binaries). If your goal is to speedup the CI, I think it also really gets time to exclude some tests --, for runs where the CI is failing during compilation, that probably doesn't matter that much, but we sometimes have to wait for CI as we cannot run too many things in parallel... |
|
I opened a discussion for a CI revision #894 |
|
Related to #763 |
|
Thanks for the investigation. Looks like spot will be recompiled any time. I think we should figure out why this is happening and whether we can prevent this. |
|
I got spot to use ccache and this helped quite a bit again. These benchmarks also have ass tests enabled:
|
|
Ok, what I gather from this are a few questios:
|
|
Regarding spot: We could
|
Things to try:
|
…om different sources. Currently we have the old every PR commit trigger and a PR label trigger. They can have different configs.
|
|
||
| # Build Storm | ||
| ############# | ||
| RUN if ! command -v ccache >/dev/null 2>&1; then \ |
There was a problem hiding this comment.
Can you write some comments here to clarify what this does (and why) for other users.
There was a problem hiding this comment.
Also can we make the usage of ccache configurable?
|
Code/build changes LGTM; @volkm can you go over the github action changes? |
volkm
left a comment
There was a problem hiding this comment.
Thanks for tackling this undertaking of revising our CI. I like the new structure and it should decrease the time for CI and make maintenance a bit easier. Great work!
I have a number of comments, I hope most are just small ones.
| set(STORM_DEBUG_SPOT ON) | ||
| set(STORM_DEBUG_SYLVAN ON) | ||
| set(STORM_WARNING_AS_ERROR ON) | ||
| # Turn off PCH for faster ccache usage on warm caches. |
There was a problem hiding this comment.
| # Turn off PCH for faster ccache usage on warm caches. | |
| # Turn off PCH for faster ccache usage on warm caches. PCH compiled files cannot be properly cached currently. |
| set_property(GLOBAL PROPERTY RULE_LAUNCH_LINK ccache) | ||
|
|
||
| if(STORM_COMPILE_WITH_PCH) | ||
| message(STATUS "STORM - PCH is enabled which will lead to significantly lower ccache hit rates. Consider disabling PCH if you plan to rebuild storm multiple times.") |
There was a problem hiding this comment.
| message(STATUS "STORM - PCH is enabled which will lead to significantly lower ccache hit rates. Consider disabling PCH if you plan to rebuild storm multiple times.") | |
| message(STATUS "STORM - PCH is enabled which will lead to significantly lower ccache hit rates. Consider disabling PCH if you plan to rebuild Storm multiple times.") |
| mark_as_advanced(CCACHE_FOUND) | ||
| if(CCACHE_FOUND) | ||
| message(STATUS "Storm - Using ccache") | ||
| set(ENV{CCACHE_SLOPPINESS} "pch_defines,time_macros,include_file_mtime,include_file_ctime") |
There was a problem hiding this comment.
Can you add an explanations for this selection here?
| "<optional>" "<ostream>" "<istream>" "<list>" "<set>" "<fstream>" "<string>" "<boost/optional.hpp>" "<boost/variant.hpp>" | ||
| "<boost/container/flat_set.hpp>" "<boost/dynamic_bitset.hpp>" "<boost/range/irange.hpp>") | ||
|
|
||
| function(storm_target_precompile_headers target) |
There was a problem hiding this comment.
Maybe move to resources/cmake/macros or to src/CMakeLists.txt?
| # Build Storm | ||
| # (This can be adapted to only build 'storm' or 'binaries' depending on custom needs) | ||
| RUN make -j $no_threads | ||
| RUN ccache --max-size=3G && ccache --zero-stats && make -j $no_threads && ccache --show-stats --verbose |
There was a problem hiding this comment.
Can we make the magic number 3G configurable?
Is showing the ccache stats necessary? Users who simply build Storm probably do not care about it.
There was a problem hiding this comment.
Why do we actually need a call to ccache? Should STORM_COMPILE_WITH_CCACHE not automatically take care of it?
| type: string | ||
| default: "Debug" | ||
| Developer: | ||
| description: "Enable developer mode (ON/OFF)" |
There was a problem hiding this comment.
In the interest of nicer handling and especially avoiding invalid input, there could be two options for improvement:
- Use custom options
"ON"/"OFF"is inputs using input type choice - Use boolean as input and convert to ON/OFF via something like
${{ input && 'ON' || 'OFF' }}
| restore-keys: | | ||
| buildtest-brew-${{ inputs.distro }}- | ||
| - name: Ensure ccache directory exists (macOS) | ||
| run: mkdir -p /Users/runner/.ccache |
There was a problem hiding this comment.
| run: mkdir -p /Users/runner/.ccache | |
| if: inputs.restore_cache | |
| run: mkdir -p /Users/runner/.ccache |
| brew update | ||
| brew install automake boost ccache cln ginac glpk hwloc libarchive xerces-c z3 | ||
| - name: Prepare ccache | ||
| run: | |
There was a problem hiding this comment.
Also guarded by save_cache or restore_cache? Also in more places later on.
| --build-arg cmake_args="-DSTORM_WARNING_AS_ERROR=ON -DSTORM_COMPILE_WITH_PCH=OFF" | ||
| # Omitting arguments developer, disable_*, cln_exact, cln_ratfunc, all_sanitizers | ||
| - name: Export ccache from Docker image | ||
| run: | |
There was a problem hiding this comment.
Should be guarded by save_cache or restore_cache? Also in more places.
| - Summary table in stdout | ||
| - CSV at <build-root>/results.csv | ||
| - Per-run ccache stats in <build-root>/results/ | ||
| - Optional traces at <build-root>/results/trace-<pch>-<cache>.json |
There was a problem hiding this comment.
| - Optional traces at <build-root>/results/trace-<pch>-<cache>.json | |
| - Optional traces at <build-root>/results/trace-<pch>-<cache>.json |


The PR CI actions where slowing me down quite a bit thus I investigated a bit into how it could be sped up. I added caching of the homebrew downloads and added ccache in all builds with a github cached ccache.