RFR(s): 8153771: Introduce MinPLABSize option

[sangheon]

Hi Thomas,

Thank you for reviewing this.

On 05/08/2017 08:45 AM, Thomas Schatzl wrote:
> Hi Sangheon,
>
> On Wed, 2017-05-03 at 16:53 -0700, sangheon wrote:
>> Hi,
>>
>> Could I get some reviews for this change of introducing MinPLABSize
>> option?
>>
>> In some cases, PLAB and TLAB requirements would be different. But
>> currently PLAB::min_size() is calculated from MinTLABSize option.
>> This proposal introduces MinPLABSize option and replaces MinTLABSize
>> with MinPLABSize at PLAB::min_size().
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8153771
>> Webrev: http://cr.openjdk.java.net/~sangheki/8153771/webrev.0
>> Testing: JPRT, TestOptionsWithRanges.java for all platforms
> - globals.hpp: why is the upper bound for MinPLABSize using SIZE_MAX
> and not max_uintx like MinTLABSize? (Note that this does not mean this
> is wrong).
I think it should be SIZE_MAX not max_uintx.
When other options' range checks added, I used max_uintx for size_t type 
because I didn't aware of SIZE_MAX.
Other options should be updated to use SIZE_MAX. I'm planning to update 
this.

>
> I also think the upper bound of SIZE_MAX/2 may result in overflows when
> calculating using it as MinPLABSize is in HeapWords and MinTLABSize in
> bytes.
Right, SIZE_MAX/2 is too large but it will be detected by constraint 
function. So there's no problem now.
But it also would be good to have more practical limit to avoid confusion.
range(1,SIZE_MAX/2/HeapWordSize) ?

>
> - commandLineFlagConstraintsGC.cpp:154: I think the min_size calculated
> here should not take AlignmentReserve into account as the calculation
> in PLAB::min_size() adds another AlignmentReserve unconditionally.
>
> Otherwise, maybe min_size should be
> align_object_size(oopDesc::header_size()) + AlignmentReserve here? (And
> similar changes done for MinTLABSize).
That was what I wanted to add but AlingmentReserve is not public, so 
used PLAB::size_required_for_allocation() method instead.

>
> The code then also needs to check that the value given by the user is a
> multiple of object_size.
>
> That is maybe another issue though :)
Ah, if you prefer we can add multiple check here.
But PLAB/TLAB related options don't check whether it's multiple of 
object_size or not.

>
> - only curious: I see that using MinTLABSize as MinPLABSize is
> annoying, but did some particular problem show up recently?
There were no problem but I wanted to correct this. :)

>
> - adding a new product flag requires a CCC/CSR request [1]
Right.
I wanted to initiated that process after getting some reviews.
Next time, I will describe about this to avoid confusion.

Let me update webrev after deciding above things.

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>
> [1] https://wiki.openjdk.java.net/display/csr/Main
>