-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Ccache #13498
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 #13498
Conversation
|
Where in the example in the first comment is emcc told to use ccache? Skimming the code I see |
| exec "$PYTHON" "$0.py" "$@" | ||
| else | ||
| unset EMCC_CCACHE | ||
| exec ccache "$0" "$@" |
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.
Why do we need to handle injecting ccache like this when other compilers such as clang and gcc don't? Can't we ask folks to use cache in the normal way (which I believe is to perpend it to compile command?)
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.
Can't we ask folks to use cache in the normal way
This is the/a normal way. Ccache supports two methods of installation: 1) explicit invocation with ccache gcc ..., and 2) automatic invocation with gcc .... See the documentation of ccache.
(which I believe is to perpend it to compile command?)
That is not the full picture. See the docs above. There are two recommended ways to install ccache, this integration is ensuring that both usages work.
E.g. man documentation: https://www.freebsd.org/cgi/man.cgi?query=ccache&sektion=1&n=1
RUN MODES
There are two ways to use ccache. You can either prefix your
compilation commands with ccache or you can let ccache masquerade as
the compiler by creating a symbolic link (named as the compiler) to
ccache. The first method is most convenient if you just want to try out
ccache or wish to use it for some specific projects. The second method
is most useful for when you wish to use ccache for all your
compilations.
Why do we need to handle injecting ccache like this when other compilers such as clang and gcc don't?
The reason that symbolic links are not used by this integration is because that would intrude with emsdk installation of either ccache or emscripten packages (say, e.g. if installing ccache package would modify installed emscripten package)
Also while Linux and macOS could use symbolic links to inject ccache, that is not well supported on Windows.
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.
My thoughts about this in general: if neither gcc nor clang require explicit in-tree modification to make ccache work I would hope that we do not need this either.
If seems that we are adding a third RUN MODE here that is emscripten-specific:
- Run explicitly via
ccache <compiler> - Run implicitly via having a
<compiler> -> ccachesymlink in your PATH - Run implicitly via add
EMCC_CACHEto the environment.
Given that mode (3) does not exist today with existing compilers I don't see why we in emscripten want to be special and add this third mode. I understand that mode (2) is hard/impossible on windows but that is pre-existing condition unrelated to emscripten, right?
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 don't agree here. Given that we can offer an easy "emsdk activate ccache" feature that just does the full integration, I don't understand why we should not do it. It gives a straightforward "fully set up out of the box" experience without manual hassles. Option 2 is a complex burden on Windows, but also on non-Windows platforms.
Also, the step (3) does not need to be something public, but an internal implementation aid to carry the integration together. I.e. we are not offering a "third run mode" here.
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.
Ok, I get that we want this to work out-of-the-box for all emsdk users including windows users.
I'd still like find a solution that doesn't involved modifying emscripten. How about this solution:
- create
emsdk/ccache/bin/emccandemsdk/ccache/bin/em++(these are bat/sh wrappers rather than symlinks) - Have
emsdk activate ccacheputemsdk/ccache/binfirst in the PATH beforeemscripten/bin.
This avoids changing emscripten at all and works more in like the symlink approach that exists today for ccache (but also happens to work on windows). It also works with older/current versions of emscripten too.
This means figuring out how to chain emsdk/ccache/bin/emcc -> emscripten/bin/emcc in the wrapper script. I don't know of the top of my head how to do that. If its too tricky I'm not going to resist this emscripten-side change too much, but it does seem wrong to me that we are hacking the compiler to be specifically aware of ccache (unlike gcc or clang).
This is done on the line
It is a way to opt in. The other method is to manually invoke with
Yes. |
| :: Remove the ccache env. var, invoke ccache and re-enter this script to take the above branch. | ||
| set EMCC_CCACHE= | ||
| ccache "%~dp0\%~n0.bat" %* | ||
| ) |
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.
These launcher scripts are tool-generated (by tools/create_entry_points.py), so it want to move forward with this I think we would want to modify that script.
Or as an alternative we would inject the cache command around the inner call to clang rather than the out call to python emcc?
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.
Injecting to the inner clang call is considerably weaker in terms of performance than injecting to the outer python emcc call.
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.
Updated the create_entry_points.py script.
|
Oh wait, sorry, I forgot we already added support for essentially this via the This can be set in the config file as This option always wraps the inner clang command. I think this is preferable as it doesn't run into the STB_IMAGE issue. |
|
The |
Very interesting. In that case maybe |
sbc100
left a comment
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.
Assuming we do land this change which is kind of like an outer version of EM_COMPILER_WRAPPER, are there other tools (such as distcc, or icecream) that might also want to use this pattern? Should we at least make it non-cache-specific?
How about EM_WRAPPER=ccache?
| shutil.copy2(bat_file, dst) | ||
|
|
||
| generate_entry_points(entry_points, os.path.join(tools_dir, 'run_python')) | ||
| generate_entry_points(compiler_entry_points, os.path.join(tools_dir, 'run_python_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.
I guess there are two new files here that need to be added?
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.
Thanks, indeed missed adding those.
| exec "$PYTHON" "$0.py" "$@" | ||
| else | ||
| unset EMCC_CCACHE | ||
| exec ccache "$0" "$@" |
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.
Ok, I get that we want this to work out-of-the-box for all emsdk users including windows users.
I'd still like find a solution that doesn't involved modifying emscripten. How about this solution:
- create
emsdk/ccache/bin/emccandemsdk/ccache/bin/em++(these are bat/sh wrappers rather than symlinks) - Have
emsdk activate ccacheputemsdk/ccache/binfirst in the PATH beforeemscripten/bin.
This avoids changing emscripten at all and works more in like the symlink approach that exists today for ccache (but also happens to work on windows). It also works with older/current versions of emscripten too.
This means figuring out how to chain emsdk/ccache/bin/emcc -> emscripten/bin/emcc in the wrapper script. I don't know of the top of my head how to do that. If its too tricky I'm not going to resist this emscripten-side change too much, but it does seem wrong to me that we are hacking the compiler to be specifically aware of ccache (unlike gcc or clang).
Have you actually tested the difference between using I should be really easy to enable too, I think just adding |
I did implement it like that first, but then revised it in the current form to avoid needing to co-maintain emcc and em++ scripts in two places at once, which would be more brittle (make a change in emscripten repo, and ccache repo needs a matching commit). Also this kind of integration is even better since now it will work without needing to resort to shell path lookup order expansion on subprocess spawns, which is very useful for build systems.
Quite a bit surprised at the friction to landing these wrappers. #12380 landed without much restrain, and arguably it modifies emscripten way more compared to this. With modifying these wrappers on the outside, the complexity remains unchanged inside the toolchain. That should be much preferable over #12380?
No, I haven't. It does not make sense to me to even compare them, it is just a waste of time. When one is utilizing a tool like ccache, it is a milliseconds competition against the added ccache operation overhead vs the time saved not invoking the compiler, and a fight to keep the time tradeoff a net positive. In such a scenario, if you have a more optimal solution (outer wrapping) vs a directly suboptimal solution (inner wrapping), it does not make sense to even look at the suboptimal solution when that optimal choice exists. The difference could be -10%, -1% or even -0.1%, but still, when using ccache is all about competing to keep the tradeoff low, it makes sense to take the optimal path without considering worse alternative. (you wouldn't ever go "meh, the time saving is only -2%, I guess I won't take it" since there are no downsides to having that -2%) The only reason why one would want to wrap clang instead of emcc is if whatever is doing the wrapping does not understand emcc, but does understand clang. That may warrant the existence of #12380. |
Indeed that was the motivation for #12380. With the goma distributed compiler that build farm knows about clang but does not know about emcc or python. For the record I really didn't want to add #12380 either :) I tried to push back at the time: #12340 (comment) I'm sorry for all the push back, I just want to avoid adding two mechanisms for doing basically the same thing. At least will you consider making it but more generic to match the existing EM_COMPILER_WRAPPER? i.e. give it a generic name that doesn't include Perhaps we should even consider renaming |
Would it be good if I renamed this to Renaming does lose the benefit that with current |
… when ccache is installed via Emscripten SDK.
|
Ran the numbers nevertheless with inner and outer compiler wrapping on timing (cache was nuked with no ccache at all:
-> outer wrapping is twice as fast in warm case (or -50% time reduction) compared to inner wrapping. The reason that outer wrapping has a +36.6% performance impact compared to inner wrapping that can be seen in the cold case is due to extra overhead of having to support dedicated emcc flags, versioning and environment variables (https://github.com/juj/ccache/blob/emscripten/src/emcc.cpp). |
Thanks for taking the time to run the numbers. I guess that also means the our wrapping only works with custom ccache fork? I think that is probably worth documenting too. I'm still not very happy with this change but it does seem like you are seeing real benefits (especially for windows users who can't use symlinks). In particular, I don't like that it has ccache baked in so its not generic (can't be used to other compiler wrapper such as distcc, icecream or goma) or it forces us to make two different (mostly identical) versions our python running scripts. But I'm not going to block it any more as I think we have been back and forth on this stuff enough. lgtm with some documentation. |
Yeah, anything wrapping emcc outside will need to understand emcc, or otherwise it will have a hard time actually doing the wrapping.
If those wrappers are emcc flags aware, then they could be used to wrap outside, but I presume currently they are not? https://github.com/juj/ccache/blob/emscripten/src/emcc.cpp contains a quite comprehensive run-through of the kinds of things a wrapper needs to be aware of. (EM_CONFIG, emscripten-version.txt, contents of .emscripten file, -s flags, etc.) A benefit of the current EMCC_CCACHE env. var is that it can still be treated as an internal implementation detail, since users are not expected to touch it. It serves as a contract between emsdk and Emscripten. However an env. var EM_OUTER_COMPILER_WRAPPER would be something to publicize. We can move EMCC_CCACHE to a EM_OUTER_COMPILER_WRAPPER later if there is another tool that can take advantage of outer wrapping, but we could not go the other way around and take such a public var private later on. Do you think from that perspective we should publicly document EMCC_CCACHE env. var for users? I would prefer to keep it internal, but I can also add docs if that's more preferable, and/or follow up with a refactor to a more general EM_OUTER_COMPILER_WRAPPER if that's something that other tools can take advantage of? |
|
If this is an internal contract between emsdk and emscripten than I'm much more happy with it. Normal non-emsdk users can always use one of the existing cache modes of operation, and we can leave this third one mostly as a convenience of emsdk+windows users. I'm more happy with that. If that is the case perhaps we can prefix it with an |
|
Yeah, that works for me. |
After emscripten-core#13498, `test_asyncify_onlylist_a` and `test_asyncify_onlylist_b` started to failing. emscripten-core#13498 introduced `if ( ... ) else ( ... )` in emcc/em++.bat, and it turns out the parentheses inside the command line can be mixed with those `if`-`else`'s parentheses themselves. This does not cause errors for every command line that contain parentheses, but when there are both a comma and parentheses, this can trigger errors. For example, `test_asyncify_onlylist_a`'s argument contains something like this: ``` 'ASYNCIFY_ONLY=["main",...,"foo(int,double)",...]' ``` Here the comma between `int` and `double` is mistaken as an item separator within the list, and `)` after `double` is consequently mistaken as ending `if`, because the whole body is within an `if`. This PR fixes the problem by assigning those argument `%*` into a variable `ARGS`, and within `if` and `else`, use it with a substitution of `)` with the escaped end-paren `^)`.
After #13498, `test_asyncify_onlylist_a` and `test_asyncify_onlylist_b` started to failing. #13498 introduced `if ( ... ) else ( ... )` in emcc/em++.bat, and it turns out the parentheses inside the command line can be mixed with those `if`-`else`'s parentheses themselves. This does not cause errors for every command line that contain parentheses, but when there are both a comma and parentheses, this can trigger errors. For example, `test_asyncify_onlylist_a`'s argument contains something like this: ``` 'ASYNCIFY_ONLY=["main",...,"foo(int,double)",...]' ``` Here the comma between `int` and `double` is mistaken as an item separator within the list, and `)` after `double` is consequently mistaken as ending `if`, because the whole body is within an `if`. This PR fixes the problem by assigning those argument `%*` into a variable `ARGS`, and within `if` and `else`, use it with a substitution of `)` with the escaped end-paren `^)`.
|
Does it also solve #12256 ? |
It does not, the plan was to not to go with an external |
|
I mean, does it solve the usecase for #12556, i.e. specify some custom script in place of emcc for emmake invocation? In theory ccache is also such a compiler wrapper script. Do I understand correctly that only |
|
As per the review comments, the conclusion was to go with an internal env. var
Yeah, one could I suppose. |
|
@juj , I like this PR. It looks that https://github.com/juj/ccache/tree/emscripten hasn't been merged in ccache, so do we have to build the ccache with your repo? And shimgen could be used for symlink on Windows, will it make it easier to use?
|
|
@mszhanyi I cannot answer your questions, but if you want to use ccache in combination with Emscripten on Windows, you might be interested in the OCI-compliant container image provided by emsdk (which can also be used when running in WSL). See for example commit kleisauke/wasm-vips@6428d47. |
This PR adds support to ccache for Emscripten. The corresponding PR for emsdk is at emscripten-core/emsdk#711 and the corresponding changes to ccache itself are at https://github.com/juj/ccache/tree/emscripten.
The general mechanism is that after activating ccache in Emscripten SDK, emcc and em++ will automatically take usage of the installed ccache tool.
Usage: