RFR (XL): 8054818: Refactor HeapRegionSeq to manage heap region and auxiliary data

[Bengt Rutisson]

Hi Thomas,

Looks good.

Bengt


On 2014-08-14 15:37, Thomas Schatzl wrote:
> Hi Mikael,
>
>    thanks for the review.
>
> On Thu, 2014-08-14 at 09:47 +0200, Mikael Gerdin wrote:
>> On Tuesday 12 August 2014 17.09.04 Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>    can I have reviews for this somewhat large refactoring change? It is
>>> about refactoring the HeapRegionSeq class to manage heap region and
>>> auxiliary data.
>>>
>>> I.e. currently HeapRegionSeq only manages the HeapRegion instances
>>> corresponding to the heap's region. The change gives HeapRegionSeq more
>>> responsibilities, encapsulating functionality related to heap memory
>>> management. This decreases the amount of responsibilities (and
>>> complexity) for the G1CollectedHeap class: decisions about how heap
>>> related memory is allocated/freed/iterated (i.e. how the heap regions
>>> are actually allocated in the heap) is removed from G1CollectedHeap.
>>>
>>> This change only changes the interface to this functionality. It is a
>>> preparatory change for JDK-8038423 "G1: Decommit memory within the
>>> heap", so the change might be slightly more extensive than really
>>> required.
>>>
>>> The RFR for JDK-8038423 will follow to look at it in context.
>>>
>>> There is another CR that renames HeapRegionSeq to HeapRegionmanager too.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8054818
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8054818/webrev/
>> It took me a while to figure out that
>>
>> 50 inline HeapWord* G1CollectedHeap::bottom_addr_for_region(uint index) const
>> {
>>    51   return _reserved.start() + index * HeapRegion::GrainWords;
>>    52 }
>>
>> Is actually referencing CollectedHeap::_reserved
>>
>> It's not clear to me if it ever differs from HeapRegionSeq::_reserved but I
>> suggest that you use that one instead.
> Fixed.
>
>> The code which does
>> heap_rs.first_part(max_byte_size);
>> looks like remnants of perm gen, where the rest of the heap_rs would be the
>> reserved memory for the perm gen.
>>
>> CollectedHeap::_reserved is a protected field but only used in a few places
>> where it's initialized by subclasses. I'll file an RFE for it to be made
>> private with a protected initialization method instead.
> Okay, thanks.
>
>>
>> heapRegionSeq.hpp
>>   177
>>   178   MemRegion committed() const { MemRegion temp(heap_bottom(),
>> heap_top()); return temp; }
>>
>> why not just
>> return MemRegion(heap_bottom(), heap_top());
>>
>>   179
>>   180   MemRegion reserved() const { MemRegion temp(heap_bottom(), heap_end());
>> return temp; }
>>
>> return MemRegion(heap_bottom(), heap_end());
>>
> Fixed.
>
>
>> I didn't look in detail at the humongous allocation code moved from G1CH to
>> HRS but I assume that you've tested this :)
> Of course :)
>
> New webrev at
> http://cr.openjdk.java.net/~tschatzl/8054818/webrev.1
> Diff:
> http://cr.openjdk.java.net/~tschatzl/8054818/webrev.0_to_1
>
> Testing:
> jprt
>
> Thanks,
>    Thomas
>