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

[David Holmes]

On 5/02/2013 7:27 AM, Chris Plummer wrote:
> 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.

In the first case we don't default to serialGC - at least not explicitly.

David
-----

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