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

John Cuthbertson john.cuthbertson at oracle.com
Mon Feb 4 23:08:24 UTC 2013


Hi Tao,

This looks OK to me. One suggestion:

Can you add an accessor routine in G1ParGCAllocBuffer:

bool retired() const { return _retired; }

and then check that all the buffers have been retired in the 
G1ParScanThreadState destructor:

~G1ParScanThreadState() {
   FREE_C_HEAP_ARRAY(size_t, _surviving_young_words_base, mtGC);
   for (int ap = 0; ap < GCAllocPurposeCount; ++ap) {
for (int pr = 0; pr < GCAllocPriorityCount; ++pr) {
       G1ParGCAllocBuffer* buffer = alloc_buffer(GCAllocPurpose(ap), 
GCAllocPriority(pr));
       assert(buffer->retired(),
              err_msg("Buffer not retired - purpose: "INT32_FORMAT", 
priority: "INT32_FORMAT,ap, pr));
       delete buffer;
}
   }
}

I understand that, at some point in the future, we're eliminating the 
G1ParGCAllocBuffer class and the _retired field but until then we may as 
well use it for safety's sake.

Thanks,

JohnC

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




More information about the hotspot-gc-dev mailing list