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

[Tao Mao]

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.
>
>>
>>>
>>> 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.
>>>>
>>>>