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

[David Holmes]

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);
        }
...

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