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

Tao Mao tao.mao at oracle.com
Sun Jun 2 04:56:04 UTC 2013


The new webrev is updated.
http://cr.openjdk.java.net/~tamao/6976350/webrev.06/

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.java.net/pipermail/hotspot-gc-dev/attachments/20130601/57035c6e/attachment.htm>


More information about the hotspot-gc-dev mailing list