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

[sangheon.kim]

Hi Jesper,

Thanks for reviewing this again.

On 04/22/2015 11:44 AM, Jesper Wilhelmsson wrote:
> 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.
Okay, I will do that.

Thanks,
Sangheon


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