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

[Tao Mao]

a new webrev updated
http://cr.openjdk.java.net/~tamao/6976350/webrev.02/

Jon, two suggestions are implemented.
(1) 2-dimensional buffers
(2) making for loop iterators more readable (But, the iterators are 
still defined as int as you can see; I have to override operator + for 
the two enum's. It's worth doing so. Maybe not.)

Bengt,
This seems a good example for discussion on refactoring, as we tried to 
initiate at the offsite meeting.
"My turn": i can imagine that you wanted to wrap up buffers_for_tenured 
and buffers_for_survived, and don't expose the array structure when you 
acquire a buffer/buffers. With the new design, what do you want to 
achieve? Clearer data structure? Readability? Extensibility? Or, other 
benefits?

Thank you all.
Tao


On 2/7/2013 9:05 AM, Jon Masamitsu wrote:
>
>
> On 02/06/13 17:11, 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.
>
> Thanks.
>
>>>
>>>>
>>>>>
>>>>> 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) {
>>   }
>> }
>
> Wouldn't it become
>
> for (GCAllocPurpose ap = GCAllocPurposeStart; ap < 
> GCAllocPurposeCount; ++ap) {
>   for (GCAllocPriority pr = GCAllocPriorityStart: pr < 
> GCAllocPriorityCount; ++pr) {
>
> Where you would have to add the start values.  For example
>
> 1745   enum GCAllocPriority {
>          GCAllocPriorityStart=0,
> 1746     GCAllocPriority1 =GCAllocPriorityStart,
> 1747     GCAllocPriority2 =GCAllocPriority1  + 1,
> 1748     GCAllocPriorityCount =GCAllocPriority2  + 1
>        };
>
>
> Jon
>>
>> 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.
>>>>>>
>>>>>>