RFR(xs): 8153201: TestOptionsWithRanges fails with -XX:OldPLABSize=2147483648
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
>>>> 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.
> In that case the changes look good. Reviewed.
>>>> Testing: JPRT, runtime/commandline JTREG tests for all platforms.
More information about the hotspot-gc-dev