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

Bengt Rutisson bengt.rutisson at oracle.com
Tue Jun 4 04:44:27 UTC 2013


On 6/3/13 11:23 PM, Tao Mao wrote:
> Thank you, Bengt. I've included your last suggestion in the final webrev.
> http://cr.openjdk.java.net/~tamao/6976350/webrev.08/
>

Looks good.

Bengt


> Tao
>
>
> On 6/3/13 1:55 PM, Bengt Rutisson wrote:
>>
>> 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/20130604/56a52cc7/attachment.htm>


More information about the hotspot-gc-dev mailing list