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

[David Holmes]

On 5/02/2013 11:27 AM, Chris Plummer wrote:
> On 2/4/13 5:06 PM, David Holmes wrote:
>> On 5/02/2013 10:57 AM, Chris Plummer wrote:
>>> On 2/4/13 2:31 PM, David Holmes wrote:
>>>> 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.
>>> As far as I can see, UNSUPPORTED_OPTION and UNSUPPORTED_GC_OPTION do the
>>> same thing, except for a slightly different warning message. Neither of
>>> them explicitly set the default to SerialGC. Also, with your approach
>>> you'll get two warnings with minimalVM builds done for embedded and
>>> for ARM.
>>
>> When !INCLUDE_ALL_GCS we call force_serial_gc() later on, forcing the
>> use of serial GC. Hence under !INCLUDE_ALL_GCS UNSUPPORTED_GC_OPTION
>> reports "defaulting to serial GC".
>>
>> But when we are simply embedded or ARM and INCLUDE_ALL_GCS is true, we
>> simply ignore UseG1GC. We do not force the use of serial GC in that
>> case, so it is wrong to use UNSUPPORTED_GC_OPTION for that case.
> Ok, to this has to do with having the warning message match what we are
> actually going to do.

Right.

>>
>>> Also, I don't see why the "if (UseG1GC) { " is needed since
>>
>> Agreed - my suggestion removes it too.
>>
>>> UNSUPPORTED_GC_OPTION already does the "if". This is a also pre-existing
>>> problem in the webrev. It got created when changing the calls from
>>> "warning" to instead call UNSUPPORTED_GC_OPTION. I think all that is
>>> needed is:
>>>
>>> #if (defined JAVASE_EMBEDDED || defined ARM || !INCLUDE_ALL_GCS)
>>> UNSUPPORTED_GC_OPTION(UseG1GC);
>>> #endif
>>
>> This bit is wrong as per the above.
> I think what we actually need then is:
>
> #if INCLUDE_ALL_GCS
> #if (defined JAVASE_EMBEDDED || defined ARM)
> UNSUPPORTED_OPTION(UseG1GC, "G1 GC");
> #endif
> #endif
>
> This solves both my concern about the double warning and your concern
> about the incorrect "defaulting to Serial GC" message when other GCs are
> supported.

Looks good to me.

>>
>>> #if !INCLUDE_ALL_GCS
>>> UNSUPPORTED_GC_OPTION(UseParallelGC);
>>> UNSUPPORTED_GC_OPTION(UseParallelOldGC);
>>> UNSUPPORTED_GC_OPTION(UseConcMarkSweepGC);
>>> UNSUPPORTED_GC_OPTION(UseParNewGC);
>>> #endif // INCLUDE_ALL_GCS
>>
> And here we need to add the following within the above #if:
>
> UNSUPPORTED_GC_OPTION(UseG1GC);
>> This can all be moved to force_serial_gc() as I suggested.
> Ok.

Thanks,
David

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