Request for review: 6976350 G1: deal with fragmentation while copying objects during GC
Tao Mao
tao.mao at oracle.com
Mon Jun 3 19:16:49 UTC 2013
new webrev:
http://cr.openjdk.java.net/~tamao/6976350/webrev.07/
Please see inline.
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/731d2b4f/attachment.htm>
More information about the hotspot-gc-dev
mailing list