RFR(xs): 8153201: TestOptionsWithRanges fails with -XX:OldPLABSize=2147483648

[Tom Benson]

Hi,
The changes look fine to me as well.

I do have to say I don't find the "consistency" argument for not just 
removing the GC type check that compelling, since we do still check raw 
ranges on unused options (EG, "-XX:+UseParallelGC 
-XX:InitiatingHeapOccupancyPercent=101" says the latter is out of 
range).  We know that's a different type of check, but probably doesn't 
look that different to the user.

In any case, I'm OK with it as is.  8^)
Tom

On 4/1/2016 3:29 AM, Bengt Rutisson wrote:
>
> Hi Sangheon,
>
> On 2016-04-01 09:07, sangheon wrote:
>> Hi Bengt,
>>
>> Thank you for looking at this.
>>
>> On 03/31/2016 10:21 PM, Bengt Rutisson wrote:
>>>
>>> Hi Sangheon,
>>>
>>> On 2016-04-01 02:05, sangheon wrote:
>>>> Hi all,
>>>>
>>>> Could I have a couple of reviews for this tiny change for 
>>>> OldPLABSize flag?
>>>>
>>>> We would face an assert complaining too large array size based on 
>>>> OldPLABSize.
>>>> And we already have a constraint for this flag checking same 
>>>> value(ThreadLocalAllocBuffer::max_size) but the constraint function 
>>>> is checking for CMS GC and G1 GC.
>>>> This change is just adding parallel gc on that check routine to 
>>>> limit both min and max.
>>>>
>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8153201
>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8153201/webrev.00
>>>
>>> Do we need to check for the GC at all? SerialGC does not use PLABs, 
>>> but that just means that those who use SerialGC should not have 
>>> OldPALBSize on their command lines. But if they do, why should it be 
>>> ok to set it to an arbitrary value? So, I would suggest just 
>>> removing the GC check all together.
>> Range/constraint validation is only working on utilized flags and 
>> there are 2 reasons.
>> Firstly, sometimes there's no way to validate. For example, 
>> ParGCStridesPerThread flag is only used at CMS GC. If other GCs want 
>> to validate it, how can we validate it? Allow all values? That is 
>> same as not checking.
>> Secondly, original JEP JDK-8059557 describes as "Non-goal 1. We will 
>> not validate arguments to flags that are not processed by the JVM". 
>> Sorry to mention the JEP again, but I'm trying to explain current 
>> implementation, I'm not refusing the enhancement if needed. :)
>>
>> In this particular case, OldPLABSize for serial GC can be limited 
>> same as parallel gc unintentionally.
>> Because PLAB max is limited by TLAB max and TLAB max is set by 
>> CollectedHeap::max_tlab_size() which serial gc also has.
>
> Ok. I see. So, the current implementation would work for SerialGC, but 
> if we improve with adding other ways for min/max size for PLABs it may 
> stop working for SerialGC. That is a valid point.
>
>>
>> However, I think it will be better to remain as is for consistency.
>
> Agreed.
>
> In that case the changes look good. Reviewed.
>
> Thanks,
> Bengt
>
>>
>> Thanks,
>> Sangheon
>>
>>
>>>
>>> Thanks,
>>> Bengt
>>>
>>>> Testing: JPRT, runtime/commandline JTREG tests for all platforms.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>
>>
>