Request for review: 6976350 G1: deal with fragmentation while copying objects during GC
Tao Mao
tao.mao at oracle.com
Fri May 31 01:31:33 UTC 2013
Sorry, the new webrev:
http://cr.openjdk.java.net/~tamao/6976350/webrev.05/
Thanks.
Tao
On 5/30/13 6:26 PM, Tao Mao wrote:
> 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/d36ac1e1/attachment.htm>
More information about the hotspot-gc-dev
mailing list