Request for review: JDK-8003581, , UseG1GC is not properly accounted for by INCLUDE_ALTERNATE_GCS

[Chris Plummer]

On 2/4/13 12:51 PM, David Holmes wrote:
> On 5/02/2013 4:43 AM, Chris Plummer wrote:
>> Hi David,
>>
>> It's not the case that only serialGC is available on embedded or ARM.
>
> Thanks. In that case it is not right to treat UseG1Gc the same as the 
> other flags and this code is incorrect:
>
> 3168 #if (defined JAVASE_EMBEDDED || defined ARM || !INCLUDE_ALL_GCS)
> 3169   if (UseG1GC) {
> 3170     UNSUPPORTED_GC_OPTION(UseG1GC);
> 3171   }
> 3172 #endif
>
> This needs to be broken into two cases, the original
>
> 3153 #if (defined JAVASE_EMBEDDED || defined ARM)
> 3154   UNSUPPORTED_OPTION(UseG1GC, "G1 GC");
> 3155 #endif
>
> plus the missing case:
>
> 3174 #if !INCLUDE_ALL_GCS
>        if (UseG1GC) {
>          UNSUPPORTED_GC_OPTION(UseG1GC);
>        }
> ...
>
I guess I don't see the significance of this different handling for the 
two cases.

Chris
>> Here's the breakdown of which GCs are available for each supported 
>> build:
>>
>>   * JDK builds (except ARM)
>>       o Detected with "#if !defined(JAVASE_EMBEDDED) && !defined(ARM)"
>>       o All GCs available
>>   * ARM JDK Build
>>       o Detected with "#if !defined(JAVASE_EMBEDDED) && defined(ARM)"
>>       o All GCs except G1
>>   * Embedded Builds (except MinimalVM)
>>       o Detected with "#if defined(JAVASE_EMBEDDED) && INCLUDE_ALL_GCS"
>>       o All GCs except G1
>>   * Embedded MinimalVM Builds
>>       o Detected with "#if defined(JAVASE_EMBEDDED) && !INCLUDE_ALL_GCS"
>>       o Only SerialGC
>>
>> Other combinations are possible (for example, a MinimalVM JDK build),
>> but the above are the supported builds. However, there are probably
>> better ways of checking the above flags to determine which GCs are
>> available, and also capture the unsupported builds. I think the
>> following is probably the simplest:
>>
>> #if INCLUDE_ALL_GCS
>>      #if defined(JAVASE_EMBEDDED) || defined(ARM)
>>     all GC's except G1 supported
>>      #else
>>     all GC's support
>>      #endif
>> #else
>>      only SerialGC supported.
>> #endif
>
> Yes, where the only serialGC supported could be a call to 
> force_serial_gc() which in turn calls the UNSUPPORTED_GC_OPTION as below.
>
> David
> ------
>
>> Chris
>>
>>
>> On 2/1/13 12:17 AM, David Holmes wrote:
>>> Hi Joe,
>>>
>>> I think you could simplify this further by deleting lines 3168 - 3186
>>> and change force_serial_gc() to do:
>>>
>>>    static void force_serial_gc() {
>>>      FLAG_SET_DEFAULT(UseSerialGC, true);
>>>      UNSUPPORTED_GC_OPTION(UseG1GC);
>>>      UNSUPPORTED_GC_OPTION(UseParallelGC);
>>>      UNSUPPORTED_GC_OPTION(UseParallelOldGC);
>>>      UNSUPPORTED_GC_OPTION(UseConcMarkSweepGC);
>>>      UNSUPPORTED_GC_OPTION(UseParNewGC);
>>>      FLAG_SET_DEFAULT(CMSIncrementalMode, false);  // special CMS 
>>> suboption
>>>    }
>>>
>>> and then:
>>>
>>> 3221 #if (!INCLUDE_ALL_GCS || defined JAVASE_EMBEDDED || defined ARM)
>>> 3222   force_serial_gc();
>>> 3223 #endif
>>>
>>> Assuming only serialGC is available on embedded or ARM.
>>>
>>> David
>>> -----
>>>
>>> On 26/01/2013 2:25 AM, Joe Provino wrote:
>>>> >  With changes from Bob and David, here is a new webrev:
>>>> >
>>>> >  http://cr.openjdk.java.net/~jprovino/8003581/webrev.01
>>>> >
>>>> >  joe
>>>> >
>>>> >  On 1/25/2013 12:44 AM, David Holmes wrote:
>>>>> >>  On 25/01/2013 4:45 AM, Bob Vandette wrote:
>>>>>> >>>  You claim to be defaulting to SerialGC but you don't set 
>>>>>> UseG1GC to
>>>>>> >>>  false?
>>>>> >>
>>>>> >>  The UNSUPPORTED_OPTION macro does that.
>>>>> >>
>>>>> >>  But I don't see anything forcing use of serialGC. And
>>>>> >>  force_serial_gc() should not be altered.
>>>>> >>
>>>>>> >>>  If you build with INCLUDE_ALL_GCS=1, the G1 code will be 
>>>>>> available
>>>>>> >>>  and be used.
>>>>> >>
>>>>> >>  Only on those platforms that support it.
>>>>> >>
>>>>> >>  David
>>>>> >>
>>>>>> >>>  Bob.
>>>>>> >>>
>>>>>> >>>  On Jan 24, 2013, at 1:38 PM, Joe Provino wrote:
>>>>>> >>>
>>>>>>> >>>>  Webrev is 
>>>>>>> here:http://cr.openjdk.java.net/~jprovino/8003581/webrev.00
>>>>>>> >>>>
>>>>>>> >>>>  It's a change to one file: arguments.cpp.
>>>>>>> >>>>
>>>>>>> >>>>  thanks.
>>>>>>> >>>>
>>>>>>> >>>>  joe
>>>>>> >>>
>>