Hello,
New webrev: http://cr.openjdk.java.net/~erikj/8062223/webrev.root.02/
Fixed the anti pattern. Also made it a fatal error to try to use ccache
if the compiler does not support the option as it's unclear if it really
works correctly then. On the other hand, that option has been around for
a long time so it will never fail.
/Erik
On 2015-01-30 11:38, Erik Joelsson wrote:
On 2015-01-30 11:24, Magnus Ihse Bursie wrote:
On 2015-01-27 14:23, Erik Joelsson wrote:
Hello,
Please review this fix cleaning up the ccache setup in configure.
This patch addresses the concerns in the following bugs:
https://bugs.openjdk.java.net/browse/JDK-8065791
https://bugs.openjdk.java.net/browse/JDK-8014074
https://bugs.openjdk.java.net/browse/JDK-8062223
Webrev: http://cr.openjdk.java.net/~erikj/8062223/webrev.root.01/
I would prefer if you rewrote the code in build-performance.m4 slightly.
We have an anti-pattern that's unfortunately too common in our
autoconf scripts, but I'm trying to eradicate it. :-)
The thing to note is that AC_ARG_ENABLE() is not affected by "if". So
even when you write:
162 if test "x$TOOLCHAIN_TYPE" = "xgcc" -o "x$TOOLCHAIN_TYPE" =
"xclang"; then
163 AC_ARG_ENABLE([ccache],
164 [AS_HELP_STRING([--enable-ccache],
165 [enable using ccache to speed up recompilations
@<:@disabled@:>@])])
the argument --enable-ccache is available for all toolchains. This is
a good thing, since we don't want to make options becoming unknown
(and thus fail) depending on circumstances. However, this behavior
cannot be deduced from the code, where it looks like it is conditional.
A better pattern is something like this:
AC_DEFUN([HANDLE_MY_OPT]), ([
# Always start by declaring the option
AC_ARG_ENABLE([myopt], ...)
if test SUPPORTED_PLATFORM; then
# .. handle option
else
AC_MSG_WARN([--myopt is ignored on this platform])
# or ERROR if more appropriate
fi
])
Right, that should be fixed.
Also, you now sets
CCACHE_SLOPPINESS=pch_defines,time_macros
only if -fpch-preprocess works with the compiler. But if it don't,
you should probably set
CCACHE_SLOPPINESS=time_macros
to keep the old behavior. Unless time_macros makes no sense if not
-fpch-preprocess is enabled?
time_macros was set only for precompiled headers. With newer versions
of ccache you also need to set pch_defines. There is no need for
either if not using precompiled headers.
/Erik
Otherwise it looks good!
/Magnus
I changed the test for ccache version to explicitly check for known
older versions that we don't want to use with precompiled headers.
The test is only done if precompiled headers are enabled, since
without precompiled headers, we don't know of any issues with ccache.
I fixed the ccache options needed to fully function with precompiled
headers in hotspot. These options will now be used if precompiled
headers are enabled. They currently aren't fully used.
I updated the documentation to reflect our current stance on ccache.
As noted in JDK-8014074, ccache works better without precompiled
headers, so I recommend turning it off when using ccache. (I
actually recommend turning it off on linux regardless.)
Some numbers building just hotspot ("make hotspot") from my machine
(E5-2665 @ 2.4GHz, 16 cores + HT) with JOBS=15 which is default:
Product/release with precompiled headers
no ccache: 03:13
empty cache: 03:37
perfect cache: 01:36
Product/release without precompiled headers
no ccache: 02:35
empty cache: 02:43
perfect cache: 01:04
/Erik
