Request for review: 6976350 G1: deal with fragmentation while copying objects during GC
Tao Mao
tao.mao at oracle.com
Mon Jun 3 21:23:59 UTC 2013
Thank you, Bengt. I've included your last suggestion in the final webrev.
http://cr.openjdk.java.net/~tamao/6976350/webrev.08/
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/20130603/1d3717d0/attachment.htm>
More information about the hotspot-gc-dev
mailing list