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

[sangheon]

Hi Tom,

Thank you for looking at this.

On 04/01/2016 07:03 AM, Tom Benson wrote:
> 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.
I agree with you and thank you for point it.
As you said, users will feel some flags are working differently(for 
above point, actually working flag on specific GC type or not) because 
they don't know about the difference between range and constraint. 
InitiatingHeapOccupancyPercent is not working as I said since it doesn't 
have constraint function, it only has range.

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

Sangheon


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