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

[David Holmes]

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.

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

>   #if !INCLUDE_ALL_GCS
>       UNSUPPORTED_GC_OPTION(UseParallelGC);
>       UNSUPPORTED_GC_OPTION(UseParallelOldGC);
>       UNSUPPORTED_GC_OPTION(UseConcMarkSweepGC);
>       UNSUPPORTED_GC_OPTION(UseParNewGC);
>   #endif // INCLUDE_ALL_GCS

This can  all be moved to force_serial_gc() as I suggested.

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