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.org/pipermail/hotspot-gc-dev/attachments/20130604/56a52cc7/attachment.htm>
More information about the hotspot-gc-dev
mailing list