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

[Mikael Gerdin]

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.

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.


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());

 181 


I didn't look in detail at the humongous allocation code moved from G1CH to 
HRS but I assume that you've tested this :)

/Mikel

> 
> Testing:
> jprt, nightly and bigapps (kitchensink, ...) with -XX:+UseG1GC
> 
> Thanks,
>   Thomas