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

Mon Jun 3 08:16:51 UTC 2013

Hi Tao,

On 6/2/13 6:56 AM, Tao Mao wrote:
> The new webrev is updated.
> http://cr.openjdk.java.net/~tamao/6976350/webrev.06/

Thanks for fixing this. I think it looks better. Still have some comments:

Line 78, int const GCAllocPriorityMax = 2;

I would prefer that this was a "private static const int" inside 
G1ParGCAllocBufferContainer. You could call it _priority_max to avoid 
the assignment in the constructor.

I think the name select_retired_buf() is a bit confusing. The way it is 
used I think the code would be more readable if we just inlined 0 in the 
code.

In G1ParScanThreadState::allocate_slow() I think we might miss retiring 
the alloc buffers, right?

We now call retire_and_set_buf() after we have we have tried the 
allcoation. If the allocation fails I think we have to retired both 
alloc buffers since both of them may have been allocated into.

I also think the name retire_and_set_buf() indicates that this method 
does too much. I would prefer to have two different methods. One 
retire_current_buf() that retires the current alloc buffer and probably 
also shifts the buffers (what adjust_priority_order() does now) and one 
that sets up a new buffer.

This will probably also be useful if we need to only retire buffers in 
the allocation failure case I described above.

Thanks,
Bengt

>
> Thanks.
> Tao
>
> On 5/30/13 10:56 PM, Bengt Rutisson wrote:
>>
>> Hi again,
>>
>> Just realized that I did this review a bit too early in the 
>> morning...before the morning coffee... One correction below.
>>
>> On 5/31/13 7:44 AM, Bengt Rutisson wrote:
>>>
>>>
>>> Hi Tao,
>>>
>>> Comments inline,
>>>
>>> On 5/31/13 3:26 AM, 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()
>>>
>>> Hm. Isn't this a bug? I think you want the method to be called 
>>> words_remaining_in_priority0_buffer() and return the remaining words 
>>> in the priority0 buffer. You call the method before you do 
>>> alloc_buf->retire_and_set_buf(), so the priority1 buffer is probably 
>>> not the one you are interested in.
>>
>> My bad. I thought the priorities were zero indexed, but because of 
>> your enum they are one indexed. So, 
>> words_remaining_in_priority1_buffer() is correct here.
>>
>> Bengt
>>
>>>
>>>>>
>>>>> 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.
>>>
>>> Let me clarify a bit why I don't like the GCAllocPriority enum. 
>>> There is really no reason to use an enum here. You are just making 
>>> code complicated without adding any semantics. You always want to 
>>> use 0-max and the order is important. This is exactly what you get 
>>> from an normal int.
>>>
>>> The enum GCAllocPurpose is different since there is no natural order 
>>> between GCAllocForTenured and GCAllocForSurvived. Thus, an enum 
>>> makes sense there.
>>>
>>> So, please remove the GCAllocPriority enum.
>>>
>>>>>
>>>>> 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.
>>>
>>> This is not the same issue as above. What I'm saying is that your 
>>> changes to GCAllocPurpose made it less readable without adding any 
>>> extra semantics. Please revert to this change.
>>>
>>>>>
>>>>> 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.
>>>
>>> OK. Good.
>>>
>>> Thanks,
>>> Bengt
>>>
>>>>>
>>>>> 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/20130603/51a17d16/attachment.htm>