Request for review: 6976350 G1: deal with fragmentation while copying objects during GC
Tao Mao
tao.mao at oracle.com
Mon Feb 11 19:48:19 UTC 2013
Got you~ I'll get back to you with a new webrev :-)
Tao
On 2/11/2013 7:39 AM, Bengt Rutisson wrote:
>
> Hi Tao,
>
> On 2/8/13 1:31 AM, 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
>> (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.
>
> Yes, this sounds about right. I think you got what I was thinking.
>
>> With the new design, what do you want to achieve? Clearer data
>> structure? Readability? Extensibility? Or, other benefits?
>
> All of the above. :) I think this will look cleaner and be more
> readable and sustainable.
>
> Best would be if you can put together an example of what it would look
> like and we can compare the webrevs.
>
> Thanks,
> Bengt
>
>>
>> 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