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

Fri May 31 05:56:14 UTC 2013

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/20130531/f9f0641d/attachment.htm>