Request for review: JDK-8003581, , UseG1GC is not properly accounted for by INCLUDE_ALTERNATE_GCS
Chris Plummer
chris.plummer at oracle.com
Mon Feb 4 17:27:35 PST 2013
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.
>
>> 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.
>
>> #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.
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
>>>>>>>>>> >>>
>>>>>>
>>>>
>>
More information about the hotspot-dev
mailing list