-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
ccache support #3761
ccache support #3761
Conversation
It might be better to provide this support through Spack's configuration file: |
It is a start! If you tell me how to query Spack's configuration in |
On a related note, for
(Details on why here: http://peter.eisentraut.org/blog/2014/12/01/ccache-and-clang-part-3/) |
What is the expected use case here? |
@citibeth : I like the idea to give the users an easy way to enable compiler caching. |
Yes, I understand that. But I don't understand why it would be helpful.
Compiler caching is useful when you compile the same thing over and over
again --- as when you are engaged in the edit/compile/run debug cycle.
Most of what Spack does is compile things once.
…On Sun, Apr 9, 2017 at 7:07 PM, Christoph Junghans ***@***.*** > wrote:
@citibeth <https://github.com/citibeth> : I like the idea to give the
users an easy way to enable compiler caching.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3761 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1cdydLth_F7BM-EeM6q7F8F-WwurBkks5ruWTAgaJpZM4M3Guh>
.
|
My understanding is similar to @citibeth 's in that we won't get anything from If that is not the case, maybe you could illustrate the difference in compile time on some big package like I also agree with @adamjstewart that such things should be configured through |
@citibeth: There are hundreds of blog posts out there showing that @davydden: The |
Thanks for the the use case; this should go in the docs when you write it
up. Specifically, two use cases: (1) Use with `spack setup`, (2) Build the
same package with many different variants.
Should this PR also try to integrate f90cache?
https://perso.univ-rennes1.fr/edouard.canot/f90cache/
…On Mon, Apr 10, 2017 at 11:37 AM, Christoph Junghans < ***@***.***> wrote:
@citibeth <https://github.com/citibeth>: There are hundreds of blog posts
out there showing that ccache is or isn't useful for a particular
package, however, from my own experience (on Gentoo) ccache comes in
handy if you need to compile similar variants of the same package (same
compiler, but different features or different patch sets). Also the time
saving depends a lot on the machine and the filesystem, where the cache is
store, it is the usual compute vs. I/O trade-off.
@davydden <https://github.com/davydden>: The libelf example above shows
that spack can useccache (cache hit (direct) 114) by simply prefixing the
compiler with the ccache binary. Sure, as discussed it would be nicer to
integrate this feature using spack setup instead of an environment
variable, but let's first decide if we want to support compiler caching.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3761 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1cd9MppyJQVuEuG07pvH7vftfJ71STks5rukyqgaJpZM4M3Guh>
.
|
lib/spack/spack/build_environment.py
Outdated
@@ -336,6 +337,10 @@ def set_build_environment_variables(pkg, env, dirty=False): | |||
env.set(SPACK_SHORT_SPEC, pkg.spec.short_spec) | |||
env.set(SPACK_DEBUG_LOG_DIR, spack.spack_working_dir) | |||
|
|||
# Find ccache binary and hand it to build environment | |||
if spack.ccache: | |||
env.set(SPACK_CCACHE_BINARY, 'ccache') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the crux, how would I detect a ccache
binary built by spack previously.
@citibeth: see my latest commit.
I will do that in another pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start, more complete docs would be great!
When set to ``true`` Spack will use ccache to cache compiles. This is | ||
useful specifically un two cases: (1) Use with ``spack setup``, (2) | ||
Build the same package with many different variants. The default is | ||
``false``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could more documentation be useful? For example... How does Spack find ccache? Are you supposed to ask Spack to build it first? How should ccache be configured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does Spack find ccache?
You tell me, how you want it (see my comment above). Currently spack just assumes ccache
is in PATH
. And I can certainly write that here.
Are you supposed to ask Spack to build it first?
Sure, if you don't have it on your system that is an option.
How should ccache be configured?
ccache
works nicely out of the box, but we can of course refer the user to the Configuration settings of man ccache
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
lib/spack/docs/config_yaml.rst
Outdated
ccache`. ``ccache`` comes with reasonable defaults for cache size | ||
and location. (See the *Configuration settings* secion of `man | ||
ccache` to learn more about the default settings and how change | ||
them.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spack install ccache
and man ccache
need double backticks around them.
lib/spack/docs/config_yaml.rst
Outdated
-------------------- | ||
|
||
When set to ``true`` Spack will use ccache to cache compiles. This is | ||
useful specifically un two cases: (1) Use with `spack setup`, (2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spack setup
needs double backticks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@citibeth: Any further comments? |
lib/spack/env/cc
Outdated
@@ -339,7 +339,11 @@ case "$mode" in | |||
args=("${args[@]}" ${SPACK_LDLIBS[@]}) ;; | |||
esac | |||
|
|||
full_command=("$command" "${args[@]}") | |||
if [[ ${lang_flags} != "F" && ${SPACK_CCACHE_BINARY} ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment explaining what ${lang_flags}!="F"
means. I think you're filtering out the Fortran compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
188b30f
to
6d73800
Compare
So, can somebody test this please, e.g.:
|
|
@healther : Hmm, strange.
The good news is that
The bad news is, there were no hits, so for some reason |
@healther: Which compiler were you using? |
The system-provided compiler
|
The number of files in the cache increases each time I uninstall and reinstall |
FYI: on my private machine (MacOS, with python3) I get the same behaviour:
|
e0cea99
to
4ae964e
Compare
@scheibelp SC17 is over, so time to merge! |
Could we please merge this? We would like to start autodeploying software with spack builds, and as long as spack is not stackable (using one |
Same case here, it would be nice to have such a feature. |
@junghans do you have any known-issues or similar that I should be aware of when using this PR? |
@healther: it should just work ;-) Can you give it another test run? So that @tgamblin or @scheibelp can finally merge this. |
I'll take a look and report back :) |
Mh, I see a massive slow down of the configure step.
with ccache the
takes significantly longer than that. Now that might be a problem of our nfs filesystem mount (which currently has some timeout problems). I'll test it also on a scratch filesystem (i.e. locally mounted, not sure about the terminology)
without ccache
and the with ccache is with a warm cache (i.e. perl was already build with it once, albeit in a different folder) and I see a lot of hits in the statistics! Any ideas what is happening here @junghans ? |
Can I get timings for the single steps from |
Okay @junghans current status:
|
It really depends on how reproducible the build is, |
Ping @tgamblin |
This feature is working for my case as well, and giving significant built time improvements. Could this be merged soon? |
Ping @tgamblin @scheibelp |
It looks like the unit tests are failing. |
lib/spack/spack/build_environment.py
Outdated
@@ -335,6 +336,13 @@ def set_build_environment_variables(pkg, env, dirty): | |||
env.set(SPACK_DEBUG_LOG_ID, pkg.spec.format('${PACKAGE}-${HASH:7}')) | |||
env.set(SPACK_DEBUG_LOG_DIR, spack.main.spack_working_dir) | |||
|
|||
# Find ccache binary and hand it to build environment | |||
if spack.config.get('ccache'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is causing the latest failing tests - it should be something like if spack.config.get('config:ccache'):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the tests are still failing based on retrieving the value of ccache
from config.yaml (you are using a .
instead of a :
to specify a property of a section)
I also have a minor request for deciding when to enable ccache support in the cc wrapper.
Other than that this looks good (which is to say I'm of a mind to finally merge it). I won't get a chance to merge this until 7/9 though.
lib/spack/spack/build_environment.py
Outdated
@@ -335,6 +336,13 @@ def set_build_environment_variables(pkg, env, dirty): | |||
env.set(SPACK_DEBUG_LOG_ID, pkg.spec.format('${PACKAGE}-${HASH:7}')) | |||
env.set(SPACK_DEBUG_LOG_DIR, spack.main.spack_working_dir) | |||
|
|||
# Find ccache binary and hand it to build environment | |||
if spack.config.get('config.ccache'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be config:ccache
vs config.ccache
(need a colon vs. a period)
lib/spack/env/cc
Outdated
@@ -342,7 +342,15 @@ case "$mode" in | |||
args=("${args[@]}" ${SPACK_LDLIBS[@]}) ;; | |||
esac | |||
|
|||
full_command=("$command" "${args[@]}") | |||
#ccache only supports C languages, so filtering out Fortran | |||
if [[ ${lang_flags} != "F" && ${SPACK_CCACHE_BINARY} ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we whitelist vs. blacklist here? e.g. if ${lang_flags} = "C" || ${lang_flags} = "CXX"
(i probably didn't get that syntax right).
I'm aware that C/CXX/F are the only possible values currently but it's generally less fragile to specify the cases where it should enable ccache.
Thanks! |
If the user sets "ccache: true" in spack's config.yaml, Spack will use an available ccache executable when compiling c/c++ code. This feature is disabled by default (i.e. "ccache: false") and the documentation is updated with how to enable ccache support
Fix #5504
and yeah