CRR: 7042285 and 7045330 : fixes and improvements to the HeapRegionSeq class (S/M)

[Stefan Karlsson]

Looks good.

Some nits:

heapRegion.hpp
  - Shouldn't G1_INVALID_HRS_INDEX be defined in heapRegionSeq.hpp?

heapRegionSeq.hpp
  - Could you move the class comment to above the class declaration?
  - type on line 147: early of doHeapRegion -> early if doHeapRegion

StefanK

On 06/01/2011 01:51 PM, Tony Printezis wrote:
> Hi,
>
> Could I please have a couple of code reviews for this change:
>
> http://cr.openjdk.java.net/~tonyp/7045330.7042285/webrev.0/
>
> The associated CRs are:
>
> 7042285: G1: native memory leak during humongous object allocation
> 7045330: G1: Simplify/fix the HeapRegionSeq class
>
> The first is the fix for the memory leak when the heap is expanded / 
> retracted aggressively that I opened for code review some time ago. We 
> are currently not reclaiming HeapRegion instances when the heap is 
> retracted, but we are creating new ones when the heap is expanded. 
> But, in all honesty, the HeapRegionSeq class is a bit of a mess so I 
> decided to expand the fix, cleanup that class, and introduce a couple 
> of small performance improvements too (all these are covered by the 
> second CR). Here's a list of the improvements (copied from the CR):
>
> a) replace the _regions growable array with a standard C array (as we 
> do not grow or shrink it so there's no point in using a growable array)
> b) avoid de-allocating / re-allocating HeapRegion instances when the 
> heap shrinks / grows (fix for 7042285)
> c) introduce fast method to map address to HeapRegion via a "biased" 
> array pointer
> d) embed the _hrs object in G1CollectedHeap, instead of pointing to it 
> via an indirection
> e) assume that all the regions added to the HeapRegionSeq instance are 
> in a contiguous address space
> f) replace int's with size_t's for region indexes (part of 6804436)
> g) remove unnecessary / unused methods
> h) rename a couple of fields to give them a more reasonable name 
> (_alloc_search_start and _seq_bottom)
> i) fix iterate_from() to do the right thing and not always start from 
> index 0 irrespective of the region passed to it
> j) add a verification method to check the HeapRegionSeq assumptions
> k) always call the wrappers for _hrs.iterate(), _hrs_length(), and 
> _hrs.at() from G1CollectedHeap, not those methods directly.
>
> In terms of volume of changes, the HeapRegionSeq class has been 
> rewritten heavily. The rest of the changes should be straightforward 
> (there are changes in G1CollectedHeap that reflect the slightly 
> different API that HeapRegionSeq now exposes).
>
> Performance-wise, I see a again modest (~2% or maybe a bit more) 
> improvement in GC times on a couple of platforms.
>
> Tony
>