RFR (S): 8073052: Rename and clean up the allocation manager hierarchy in g1Allocator.?pp

[Sangheon Kim]

Hi Thomas,

New version looks good.

Just copyright year of G1CollectedHeap.java needs update.

[openjdk id: sangheki]
Thanks,
Sangheon

On 03/18/2015 03:37 AM, Thomas Schatzl wrote:
> Hi Stefan, Kim,
>
>    could you look at the updated CR again? Also, I need a Reviewer to
> look at this.
>
> Thanks,
>    Thomas
>
> On Mon, 2015-03-09 at 12:02 +0100, Thomas Schatzl wrote:
>> Hi Stefan,
>>
>>    thanks for the review.
>>
>> There is a new webrev at
>> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.2 (full)
>> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.1_to_2 (diff)
>>
>> Further comments inline:
>>
>> On Fri, 2015-03-06 at 10:00 +0100, Stefan Johansson wrote:
>>> Hi Thomas,
>>>
>>> On 2015-03-05 15:58, Thomas Schatzl wrote:
>>>> Hi all,
>>>>
>>>> On Wed, 2015-03-04 at 16:28 +0100, Thomas Schatzl wrote:
>>>>> Hi all,
>>>>>
>>>>>     can I have reviews for the following change that does some renamings
>>>>> in the allocation related class hierarchy, also adding a few lines of
>>>>> documentation.
>>>>>
>>>>> This change is intentionally limited to renames to keep it simple, in a
>>>>> subsequent patch there will be more cleanup changes, moving methods
>>>>> around.
>>>>> Also, if there is demand, I am open to rename the files to something
>>>>> different (suggestions?) in a follow-up change (to not confuse the
>>>>> webrev tool too much).
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8073052
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.00/
>>>>> Testing:
>>>> I talked with Stefan Johansson a bit about the change, with the result
>>>> that we keep the name of G1Allocator, since in the upcoming changes (see
>>>> below for more) that class is supposed to handle all allocations within
>>>> the current allocation regions.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8073052/webrev.01
>>> Generally change looks good, but I agree with Kim that PLABAllocator
>>> could still use the G1 prefix for consistency. Some other comments:
>> Done.
>>
>>> ---
>>> g1CollectedHeap.hpp:
>>> +  // Allocates a new heap region instance.
>>> +  virtual HeapRegion* new_heap_region(uint hrs_index, MemRegion mr);
>>>
>>> No need to have this one virtual, no one extends G1CollectedHeap.
>> Done.
>>
>>> ---
>>> vmStructs_g1.hpp:
>>> Since _summary_bytes_used have been moved in the native code this needs
>>> to be reflected in the Java agent as well. You need to move the
>>> functionality from G1Allocator.java to G1CollectedHeap.java.
>> Done.
>>
>> Thanks,
>>    Thomas
>>
>>
>