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

[Tao Mao]

Please see inline.

Thanks.
Tao

On 5/30/13 5:53 AM, Bengt Rutisson wrote:
>
> 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().
Done.
changed to words_remaining_in_priority1_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
>     }
It's more like code style issue. In fact, it was done this way according 
to the Jon's earlier suggestion. Second, if we want to change #buffer to 
3 (it wont give more benefits to define more than that number), we only 
need to add one more enum value, i.e. GCAllocPriority3.
>
> I find the name G1MultiParGCAllocBuffer confusing since it is not 
> inheriting G1ParGCAllocBuffer. Maybe G1AllocBufferContainer or 
> something like that would make more sense?
Done.
Changed to G1ParGCAllocBufferContainer
>
> 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.)
See 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.)
In G1ParGCAllocBuffer, we need the field _retired to handle buffer 
allocation failure. This is handled differently for other collectors. 
For example, ParScanThreadState::alloc_in_to_space_slow in ParNew. Thus, 
moving the _retired field up to its super class will involve additional 
efforts. This is supposed to be investigated in another CR JDK-7127700.
>
> A couple of minor things:
>
> 1800     if (finish_undo != true) ShouldNotReachHere();
>
> should be:
>
> 1800     if (!finish_undo) ShouldNotReachHere();
Done.
>
>  Please add spaces before and after "=" here:
> 1804     size_t result=0;
Done.
>
> There are two spaces after "=" here:
> 1812     G1ParGCAllocBuffer* retired =  
> _priority_buffer[GCAllocPriority1];
Done.
>
> 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.
Done.
>
> 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/48d6e838/attachment.htm>