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

[Bengt Rutisson]

On 1/24/14 11:31 AM, Jesper Wilhelmsson wrote:
> Hi,
>
> Bengt Rutisson skrev 24/1/14 8:36 AM:
>>
>> 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().
>
> I have a change where I merge GenCollectorPolicy with 
> TwoGenerationalCollectorPolicy. In that work I also removed a lot of 
> the imaginary support for more than two generations. It all ties 
> together...
>
> It's not done yet and currently it's not at the top of my priority 
> list, but let me know if you think I should give this a few more hours 
> and get it in sooner rather than later.

Thanks Jesper. I don't think this current change makes that work more 
important than it was before. But it does sound like a nice fix to get in.

Bengt


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