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

[Thomas Schatzl]

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