RFR: JDK-8062223: Upgrading to ccache 1.3.10 disables the use of ccache

[Erik Joelsson]

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
>>
>