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

[Jon Masamitsu]

On 1/23/2014 11:36 PM, Bengt Rutisson wrote:
>
> 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().

Fair enough.

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

Also OK.

Jon

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