RFR (S): 8073204: Determining the desired PLAB size adjusts to the the number of threads at the wrong place

[Jesper Wilhelmsson]

Looks good!

It would be nice if you aligned the comments in plab.hpp (add a space after 
_desired_net_plab_sz; and move the others out). No need for a new webrev if you 
do that change.

Thanks,
/Jesper


sangheon.kim skrev den 22/4/15 20:37:
> Hi Ramki and Jesper,
>
> Here's updated webrev which contains your comments.
> - Renamed to '_desired_net_plab_sz'.
> - Changed comments to 'Assumed to have'.
>
> And this webrev is created after a rename of G1ParGCAllocBuffer to G1PLAB.
>
> webrev:
> http://cr.openjdk.java.net/~jwilhelm/8073204/webrev.02/
>
> Thanks,
> Sangheon
>
>
> On 04/08/2015 01:28 PM, Sangheon Kim wrote:
>> Hi Jesper,
>>
>> Thank you for the review.
>>
>> On 04/08/2015 12:57 PM, Jesper Wilhelmsson wrote:
>>> Hi Sangheon,
>>>
>>> Looks good in general. Just minor comments:
>>>
>>> In parGCAllocBuffer.cpp
>>>
>>> +  // Assume to have 1 gc worker thread
>>> +  size_t recent_plab_sz = used / (target_refills * 1);
>>>
>>> I don't see the point of keeping the "* 1" here, or the parenthesis. The
>>> comment already says that we assume one thread. I think the comment should
>>> say "Assumed to have" or "We assume that we have".
>> Okay, I will fix this.
>>
>>>
>>> Since you were discussing naming with Ramki I'll add that I don't see the
>>> point in saving two characters by shortening size to sz. I'd prefer if size
>>> was spelled out throughout the code. That may be too much to change in this
>>> patch, but if you decide to change the name anyway please consider this as well.
>> Yes. Thomas and I discussed from other email about changing name. But as you
>> said, it's too much to cover within this CR so I would prefer to separate to
>> another CR.
>>
>> Thanks,
>> Sangheon
>>
>>
>>>
>>> Thanks,
>>> /Jesper
>>>
>>>
>>> Sangheon Kim skrev den 6/4/15 23:40:
>>>> Hi all,
>>>>
>>>> Please review this change to determine the desired PLAB size for current gc
>>>> worker threads.
>>>>
>>>> Currently we calculate an optimal PLAB size with current number of gc workers.
>>>> When the number of workers changes dynamically
>>>> (-XX:+UseDynamicNumberOfGCThreads), the desired PLAB size returned(by
>>>> desired_plab_sz()) is still tuned to the number of gc workers that has been
>>>> used
>>>> previously.
>>>>
>>>> This change is first calculate the desired PLAB value for a single gc worker
>>>> and
>>>> then return desired PLAB size according to the current number of threads.
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8073204
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sangheki/8073204/webrev.01
>>>>
>>>> Test:
>>>> JPRT
>>>>
>>>> Thanks,
>>>> Sangheon
>>
>