RFR(xs): 8152118: MinTLABSize should be less than TLAB max

[Bengt Rutisson]

Hi Sangheon,

On 2016-03-18 18:28, sangheon wrote:
> Hi Bengt,
>
> Thanks for reviewing this.
>
> On 03/18/2016 12:15 AM, Bengt Rutisson wrote:
>>
>> Hi Sangheon,
>>
>> On 2016-03-18 06:43, sangheon wrote:
>>> Hi all,
>>>
>>> Could I have a couple of reviews for this tiny change?
>>>
>>> MinTLABSize doesn't have upper limit now and this change is 
>>> proposing to be less than or equal to 
>>> ThreadLocallAllocBuffer::max_size().
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8152118
>>> Webrev: http://cr.openjdk.java.net/~sangheki/8152118/webrev.00/
>>
>> I think this looks good.
> Thanks.
>
>>
>> However, I have a related question. When I set up the min tlab size 
>> high I get some issues reported from other constraint functions:
>>
>> $ java  -XX:MinTLABSize=16m -XX:G1HeapRegionSize=32m -version
>> YoungPLABSize (4096) must be greater than or equal to ergonomic PLAB 
>> minimum size (2097152)
>> OldPLABSize (1024) must be greater than or equal to ergonomic PLAB 
>> minimum size (2097152)
>> Error: Could not create the Java Virtual Machine.
>> Error: A fatal exception has occurred. Program will exit.
>>
>> I don't think this is a problem with the patch that you are proposing 
>> now, but I thought I should mention it. Do you think this is 
>> something we would like to fix in the argument parsing (like 
>> adjusting the PLAB max to be at least PLAB min) or should we just let 
>> the users set up the PLAB sizes if they use large TLAB sizes?
>>
>> This is a pre-exiting problem. Even without your patch I get the same 
>> behavior. So, you can consider your proposed patched reviewed.
> Right, we have this problem when we introduced range/constraint for 
> command-line flags (JDK-8059557: JEP 245: Validate JVM Command-Line 
> Flag Arguments).
> And adjusting the flag during argument processing was out-of-scope. 
> (Non-goal: second item)

Thanks for this background information. But just because it was out of 
scope for JEP 245 doesn't mean that we can't fix it now if we want, 
right? :)

> Your opinion seems like adjusting related flags, am I correct? i.e. We 
> set 'MinTLABSize' but the error happened related flags(YoungPLABSize 
> and OldPLABSize). And then, I think it would be better to let the 
> users set-up them because it will be hard to adjusting related flags. 
> What extends we would decide candidate flags to adjust.
>
> But I agree with you that it would be annoying for the users.

Yes, argument consistency is really difficult to get completely right. 
You can spend a lot of time on it and still miss some border cases. I 
just thought that in this case it looked simple enough. We should 
probably not select PLAB sizes that smaller than the min PLAB size.

But I haven't looked at the code. Just the error messages. So, I'll 
leave it to you if you want to pursue this further by preparing a patch 
or just filing an RFE. Or just leave it as is. Either way is fine with 
me, I just wanted to mention it since I noticed it.

Also, just to be clear. I find you changes for "8152118: MinTLABSize 
should be less than TLAB max" to be good. Consider them reviewed.

Thanks,
Bengt

>
> Thanks,
> Sangheon
>
>
>>
>> Thanks,
>> Bengt
>>
>>
>>> Testing: JPRT, TestOptionsWithRanges.java for all platforms.
>>>
>>> Thanks,
>>> Sangheon
>>
>