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