RFR (M): JDK-8030177: G1: Enable TLAB resizing

[Bengt Rutisson]

Hi Jon,

Thanks for looking at this!

On 1/24/14 12:36 AM, Jon Masamitsu wrote:
> Bengt,
>
> http://cr.openjdk.java.net/~brutisso/8030177/webrev.01/src/share/vm/memory/genCollectedHeap.cpp.frames.html 
>
>
> An opportunity to remove _gens[] in another place?  Since we
> really only have two generations in GenCollectedHeap.
>
>  936 size_t GenCollectedHeap::tlab_used(Thread* thr) const {
>  937   size_t result = 0;
>  938   for (int i = 0; i < _n_gens; i += 1) {
>  939     if (_gens[i]->supports_tlab_allocation()) {
>  940       result += _gens[i]->tlab_used();
>  941     }
>  942   }
>  943   return result;
>  944 }

I like this idea, but I would prefer to make this as a general change 
where we remove the generations array. For now I would prefer that 
GenCollectedHeap::tlab_used() looks as similiar at possible to the other 
methods in GenCollectedHeap, such as 
GenCollectedHeap::supports_tlab_allocation(), 
GenCollectedHeap::tlab_capacity() and 
GenCollectedHeap::unsafe_max_tlab_alloc().

>
> Can tlab_used() get Thread *thr from myThread() instead
> of passing it in?   It's somewhat confusing when a parameter
> is passed in and then ignored a lot of the time.

I agreee. Currently it is only MutableNUMASpace that actually use the 
thread parameter. Again, my personal view is that we should treat 
tlab_used() similarly to the other tlab_* methods and I would prefer to 
do changes like this as separate fixes.

Thanks,
Bengt

>
> Rest looks good.
>
> Jon
>
> On 01/23/2014 01:27 AM, Bengt Rutisson wrote:
>>
>> Hi all,
>>
>> I went through this change with Stefan Karlsson. Here is an updated 
>> webrev based on his suggestions:
>>
>> http://cr.openjdk.java.net/~brutisso/8030177/webrev.01/
>>
>> Here is a webrev with just the changes that Stefan suggested:
>>
>> http://cr.openjdk.java.net/~brutisso/8030177/webrev.01_delta/
>>
>> Thanks,
>> Bengt
>>
>>
>> On 2014-01-13 14:09, Bengt Rutisson wrote:
>>>
>>> Hi Thomas,
>>>
>>> On 1/13/14 1:14 PM, Thomas Schatzl wrote:
>>>> Hi,
>>>>
>>>> On Thu, 2014-01-09 at 20:32 +0100, Bengt Rutisson wrote:
>>>>> Hi everyone,
>>>>>
>>>>> Can I have a couple of reviews for this change?
>>>>>
>>>>> http://cr.openjdk.java.net/~brutisso/8030177/webrev.00/
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8030177
>>>>>
>>>>> Unlike the other collectors G1 did not use dynamic sizes for the 
>>>>> TLABs
>>>>> that the Java threads use for allocation. That means that the TLABs
>>>>> when using G1 were all fixed size and very small. This patch adds
>>>>> dynamic sizing of TLABs to G1 too. In performance testing this has
>>>>> shown a 2-3% improvement across a range of benchmarks.
>>>>>
>>>>> One difference compared to other collectors is that the maximum TLAB
>>>>> size in G1 has to be limited by the region size and not by the eden
>>>>> size. Also, for the other collectors the used space for TLAB
>>>>> allocation was deducted from the maximum TLAB size because this was
>>>>> essentially what was left in eden. That is an assumption that 
>>>>> happened
>>>>> to work out, but it kind of breaks the abstraction. So, the method
>>>>> CollectedHeap:: tlab_used() has been added to be more consistent 
>>>>> about
>>>>> the TLAB abstraction and to make it more straight forward to 
>>>>> implement
>>>>> in G1.
>>>> threadLocalAllocBuffer.cpp:
>>>>
>>>> line 83: typo:  allcoation -> allocation
>>>>        // These allcoation should ideally not be counted but since 
>>>> it is
>>>> not possible
>>>>
>>>> Looks good. It's okay for me to just fix this without a re-review ;-)
>>>
>>> Thanks! I fixed the typo. :)
>>>
>>> Bengt
>>>
>>>
>>>>
>>>> Thomas
>>>>
>>>>
>>>
>>
>