Request for review: 6976350 G1: deal with fragmentation while copying objects during GC
Jon Masamitsu
jon.masamitsu at oracle.com
Fri Feb 8 15:37:06 UTC 2013
On 2/7/2013 4:31 PM, Tao Mao wrote:
> a new webrev updated
> http://cr.openjdk.java.net/~tamao/6976350/webrev.02/
>
> Jon, two suggestions are implemented.
> (1) 2-dimensional buffers
Thanks.
> (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.)
This looks fine. I leave it to you if you want to override the + operator.
Finish your discussions with Bengt, but overall, looks good.
Jon
>
> 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.
>>>>>>>
>>>>>>>
More information about the hotspot-gc-dev
mailing list