Request for review: 6976350 G1: deal with fragmentation while copying objects during GC

[Bengt Rutisson]

Hi Tao,

I think the code is a little bit confused about whether 
G1MultiParGCAllocBuffer can handle an arbitary number of AllocPriorites 
or just 2. All the for loops indicate that we think we might want to 
change from 2 to a larger number in the future. But the naming of a 
method like words_remaining_in_retired() indicate that there can only be 
one retired region. With the current implementation I think 
words_remaining_in_retired() should be called something like 
words_remaining_in_priority0_buffer().

I think it would be good to make this code truly general with respect to 
the number of priorities. We can then use 2 as default, but make sure 
that the code works with more priorities. To do that I think we should 
remove the enum GCAllocPriority and instead have a field in 
G1MultiParGCAllocBuffer that contains the maximum number of priorities. 
I think that will make the code more general and easier to read. The for 
loops would look like:

     for (int pr = 0; pr < _max_priorities; ++pr) {
       // do stuff
     }

I find the name G1MultiParGCAllocBuffer confusing since it is not 
inheriting G1ParGCAllocBuffer. Maybe G1AllocBufferContainer or something 
like that would make more sense?

I don't understand why you added initialization values to 
GCAllocPurpose. You are only using the values that are default in C++ 
anyway: 0, 1, 2. At least if you avoid adding the GCAllocPurposeStart 
value. I think it was more readable before your change. (The same 
argument holds for GCAllocPriority, but I prefer to remove that enum all 
together as I described above.)

Have you considered moving the _retired field from G1ParGCAllocBuffer to 
ParGCAllocBuffer instead of making the retire() method virtual? (I do 
think your change to virtual is needed in the current code, so good 
catch! But I think it might make sense to have the logic of 
G1ParGCAllocBuffer::retire() in ParGCAllocBuffer::retire() instead.)

A couple of minor things:

1800     if (finish_undo != true) ShouldNotReachHere();

should be:

1800     if (!finish_undo) ShouldNotReachHere();

  Please add spaces before and after "=" here:
1804     size_t result=0;

There are two spaces after "=" here:
1812     G1ParGCAllocBuffer* retired = _priority_buffer[GCAllocPriority1];

Also, in g1CollectedHeap.hpp you have updated the copyright year but not 
in parGCAllocBuffer.hpp. As you know we in the GC team have agreed not 
to update the copyright year. If you absolutely want to do it I think 
you should do it the same way in all files.

Thanks,
Bengt

On 5/24/13 1:31 AM, Tao Mao wrote:
> Can I have a couple of reviewers please?
>
> Thank you.
> Tao
>
> On 5/20/13 5:11 PM, Tao Mao wrote:
>> Hi all,
>>
>> a new webrev
>> http://cr.openjdk.java.net/~tamao/6976350/webrev.04/
>>
>> diff:
>> (1) John Cuthbertson and I figured out the way to handle "retire an 
>> old buffer, allocate and set a new one" and it can possibly make the 
>> usage of allocation buffer a little more efficient.
>> (2) Make the assertion as John suggested and provide some harness 
>> (i.e. making retire() a virtual function) to cope with it.
>>
>> Thanks.
>> Tao
>>
>> On 5/15/13 10:58 PM, John Cuthbertson wrote:
>>> Hi Tao,
>>>
>>> This looks excellent. One minor question: does it make sense to 
>>> assert that each buffer has been retired? It might save a missed 
>>> call to PSS::retire_alloc_buffers. I'll leave the decision to you. 
>>> Ship it.
>>>
>>> JohnC
>>>
>>>
>>> On 5/14/2013 3:06 PM, Tao Mao wrote:
>>>> To the open list:
>>>>
>>>> new webrev:
>>>> http://cr.openjdk.java.net/~tamao/6976350/webrev.03/
>>>>
>>>> I took suggestion from many reviewers into consideration and came 
>>>> up with the current cleaner solution.
>>>>
>>>> Thank you.
>>>> Tao
>>>>
>>>>
>>>> On 5/14/13 2:26 PM, Jon Masamitsu wrote:
>>>>> What's the status of this review?
>>>>>
>>>>> The last mail I  could find in my mail boxes was a comment
>>>>> from Thomas.
>>>>>
>>>>> Jon
>>>>>
>>>>> On 1/28/13 12:21 PM, Tao Mao wrote:
>>>>>> 6976350 G1: deal with fragmentation while copying objects during GC
>>>>>> https://jbs.oracle.com/bugs/browse/JDK-6976350
>>>>>>
>>>>>> webrev:
>>>>>> http://cr.openjdk.java.net/~tamao/6976350/webrev.00/
>>>>>>
>>>>>> changeset:
>>>>>> Basically, we want to reuse more of par-allocation buffers 
>>>>>> instead of retiring it immediately when it encounters an object 
>>>>>> larger than its remaining part.
>>>>>>
>>>>>> (1) instead of previously using one allocation buffer per GC 
>>>>>> purpose, we use N(=2) buffers per GC purpose and modify the 
>>>>>> corresponding code. The changeset would easily scale up to 
>>>>>> whatever N (though Tony Printezis suggests 2, or 3 may be good 
>>>>>> enough)
>>>>>>
>>>>>> *(2) Two places of cleanup: allocate_during_gc_slow() is removed 
>>>>>> due to its never being called.
>>>>>>                                               access modifier 
>>>>>> (public) before trim_queue() is redundant.
>>>>>>
>>>>>>
>>>>>
>>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130530/5f221489/attachment.htm>