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

Bengt Rutisson bengt.rutisson at oracle.com
Mon Jun 3 20:55:07 UTC 2013


Tao,

On 6/3/13 9:16 PM, Tao Mao wrote:
> new webrev:
> http://cr.openjdk.java.net/~tamao/6976350/webrev.07/

This looks good to me. Thanks for working through all of these iterations!

There are superfluous spaces on these lines before _priority_buffer[0]:

1803     G1ParGCAllocBuffer* retired_and_set =  _priority_buffer[0];
1812     G1ParGCAllocBuffer* retired_and_set =  _priority_buffer[0];

Other than that it looks good!

>
> Please see inline.

Thanks for the detailed description about retiring alloc buffers. It was 
very helpful.

Bengt

>
> Thanks.
> Tao
>
> On 6/3/13 1:16 AM, Bengt Rutisson wrote:
>>
>> 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.
> Done.
>
>>
>>
>> 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.
> Inlining done.
>
>>
>>
>> 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.
> The current implementation is right.
>
> (1) Even if the code reaches the point where it need to allocate a new 
> buffer and fails, the old buffer is still usable. There's no reason we 
> should retire it entirely.
>
> In addition, I designed to keep the buffer when the allocation fails 
> so that the code doesn't need additional checkings in order to make 
> sure the buffer is still valid, when trying to allocate a new object 
> and to retire it per se.
>
> In fact, the current implementation simplifies the logic of object 
> allocation in the buffer and retiring buffers if you get it.
>
> (2) Please check out the function retire_alloc_buffers(). It guards 
> the clearing of all buffers in the end of copying phase, so we don't 
> have to worry about that part.
>
> (3) A subtle benefit to mention: I still keep the buffer when the 
> attempted allocation fails such way that we hope that the buffer may 
> be allocated again to contain a new "smaller" object. It can happen to 
> improve heap efficiency.
>
>>
>> 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.
> I think it's a single operation and can't be separated. If we 
> separated this function, we would end up with exposing unnecessary 
> details to the caller. Also, the order of retire(), set_buf(), 
> set_word_size() and adjust_priority_order() matters. I don't want to 
> the caller to handle this rather than handle the ordering issue in 
> class G1ParGCAllocBufferContainer.
>
>>
>> This will probably also be useful if we need to only retire buffers 
>> in the allocation failure case I described above.
> As I mentioned above, retiring buffers upon the allocation failure 
> would introduce additional complexity to handling the buffer usage 
> and, even, retiring process itself. Please also refer to the last 
> third comments 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.java.net/pipermail/hotspot-gc-dev/attachments/20130603/184575d1/attachment.htm>


More information about the hotspot-gc-dev mailing list