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

[Bengt Rutisson]

Hi Tao,

One comment inline.


On 2/7/13 2:11 AM, Tao Mao wrote:
> Please see inline, Jon. Thanks.
> Tao
>
> On 2/4/2013 9:46 PM, Jon Masamitsu wrote:
>>
>>
>> On 2/4/2013 12:56 PM, Tao Mao wrote:
>>> a new webrev created (I need more reviews!)
>>> http://cr.openjdk.java.net/~tamao/6976350/webrev.01/
>>>
>>> I took Jon's suggestion for bracketing for loops.
>>>
>>> For the rest of suggestions, I thought of them when coding but I 
>>> also had another concern to myself not to have them. So, I'd like to 
>>> discuss before I do further changes.
>>>
>>> Please see inline and provide some insights on my concerns.
>>>
>>> Thank you.
>>> Tao
>>>
>>> On 2/1/2013 2:58 PM, Jon Masamitsu wrote:
>>>>
>>>> http://cr.openjdk.java.net/~tamao/6976350/webrev.00/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.frames.html 
>>>>
>>>>
>>>> line 4386
>>>>
>>>> Please add the brackets for the outer for loop.  I'm not used to 
>>>> seeing it
>>>> without the brackets even though they are not strictly needed.
>>>>
>>>> http://cr.openjdk.java.net/~tamao/6976350/webrev.00/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp.frames.html 
>>>>
>>>>
>>>> line 1821 same comment about brackets for for-statement.
>>>>
>>>> line 1869
>>>>
>>>> You always retire alloc_buf1.  Did you consider retiring the buffer 
>>>> that has the
>>>> less words_remaining?
>>> Why not to leverage info of words_remaining:
>>> 1) simplicity.
>>> 2) The current mechanism to use the alloc_buffers in order (in 
>>> routine allocate() ) makes alloc_buf1 stochastically filled up first.
>>> 3) The current changeset will make the situation better. Do we want 
>>> an additional overhead (getting words_remaining) to trade for a 
>>> little more benefits? Maybe...
>>
>> Ok.  In practical  terms you're probably right that the benefit would 
>> be small.
>>
>>>
>>>>
>>>> line 1762
>>>>
>>>> Can you make _alloc_buffers two dimensional?  Then you wouldn't 
>>>> need the
>>>> buf_idx() method.
>>> I rarely saw any 2D array implementation at least within GC code 
>>> base. I didn't know whether this was a coincidence or some 
>>> "convention". So I was some reluctant to do so.
>>
>> I don't think there is a convention that says we shouldn't use 
>> multidimensional arrays.
>>
>> If I'm wrong, someone please let me know.
> I will make the array two-dimensional, if no one opposes to it or any 
> convention rules here.

I just had a quick look at this change so I may be wrong (I'll try to 
look more closely soon). But I think that rather than doing a two 
dimensional array we could do a more object oriented solution. There 
could be an object to keep track of the allocation buffers in priority 
order. And then we have one such object per allocation purpose.

I need to spend some more time on this to have a clearer design 
suggestion, but I thought that mention it. Maybe you come up with an 
object oriented design if you think about it a bit.

Otherwise this looks good. Thanks for fixing it!

BTW, do we have any performance data on this change? I can imagine that 
it could be very useful, but it would be interesting to see some data on it.

Thanks,
Bengt

>>
>>>
>>>>
>>>> GCAllocPriority is an enum but you use them as integers in the for 
>>>> loops.
>>>> Can you make the for loop index variable an enum GCAllocPriority?
>>> Making enum GCAllocPriority as for loop index is not a big deal. But 
>>> the problem is that, for consistency, we should make GCAllocPurpose 
>>> as for loop index as well, which has values of GCAllocForTenured and 
>>> GCAllocForSurvived, and don't make as much sense as GCAllocPriority1 
>>> and GCAllocPriority2 do.
>>
>> True.   GCAllocPurpose's don't have an order the way the GCAllocPriority
>> but I think the code ends up more readable.
> Then, the current two-layer for loop
>
> for (int ap = 0; ap < GCAllocPurposeCount; ++ap) {
>   for (int pr = 0; pr < GCAllocPriorityCount; ++pr) {
>   }
> }
>
> would become
>
> for (int ap = GCAllocForTenured; ap < GCAllocPurposeCount; ++ap) {
>   for (int pr = GCAllocPriority1; pr < GCAllocPriorityCount; ++pr) {
>   }
> }
>
> Do you still see the initial value for iterator "ap" is more readable? 
> Or, you get puzzled about whether it's the true starting value?
>
>>
>> Jon
>>
>>>
>>>>
>>>> Jon
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 1/28/2013 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.
>>>>>
>>>>>