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

[Bengt Rutisson]

Hi Thomas,

Thanks for looking at this again!

On 2014-01-23 11:35, Thomas Schatzl wrote:
> On Thu, 2014-01-23 at 10:27 +0100, 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/
> A few more cleanup requests:
>
>   - G1CollectedHeap::unsafe_max_tlab_alloc(Thread* ignored) const could
> use the new max_tlab_size() method too instead of doing the same
> calculation. It seems to return a just too large value too..

Good point. Fixed.

>
>   - the comment for G1CollectedHeap::max_tlab_size() could probably
> improved, e.g. "TLABs should not contain..." instead of "TLABs should
> not be containing..."

Fixed.

>
>   - in G1CollectedHeap::tlab_capacity(Thread), why not use
> g1_policy->young_list_target_length() - young_list()->length() as a
> better approximation of the remaining amount of space?

Actually it should not return the remaining space but the total space. 
So, the current implementation is correct.

>
>   - ThreadLocalAllocBuffer::set_max_size() does not seem to be used
> anywhere any more. Which makes the assert() in max_size() fail always.
> (I grepped
> http://cr.openjdk.java.net/~brutisso/8030177/webrev.01/hs-gc9.patch and
> the only occurrence of set_max_size() is its declaration).
>
> Is the patch complete?

Sorry. This was lost when I was doing some mq-tricks to get the webrevs 
together. Thanks for catching it!

The call should be in Universe::initialize_heap(). I added it back now.

>
>   - not sure why the change makes G1CollectedHeap::young_list() const
> while adding young_list_target_lenght() non-const. Any reasons?

It is because I now use it in G1CollectedHeap::tlab_used(), which is 
const and can only call const methods.

>
>   - could the patch initialize the other members of
> ThreadLocalAllocBuffer in
> ThreadLocalAllocBuffer::ThreadLocalAllocBuffer() too?

I'd prefer not to do this as part of this change. The constructor now 
has this comment:

     // do nothing.  tlabs must be inited by initialize() calls

So, I think it might be better to move all initialization from 
initialize() to the constructor in a separate change.

New webrev:
http://cr.openjdk.java.net/~brutisso/8030177/webrev.02/

Thanks,
Bengt
>
> Thanks,
>    Thomas
>