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

Tom Benson tom.benson at oracle.com
Fri Apr 1 14:03:11 UTC 2016

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

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

More information about the hotspot-gc-dev mailing list