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

[Tony Printezis]

Stefan,

Thanks for the code review. Please see answers inline:

On 06/01/2011 03:03 PM, Stefan Karlsson wrote:
> Looks good.
>
> Some nits:
>
> heapRegion.hpp
>  - Shouldn't G1_INVALID_HRS_INDEX be defined in heapRegionSeq.hpp?

You probably saw in the follow-up change that I use G1_INVALID_HRS_INDEX 
to initialize the _hrs_index field of the HeapRegion class. So I thought 
it was pointless to put it in one file and then move it to another one. 
BTW, I'm not 100% happy with the G1_INVALID_HRS_INDEX name. Would 
G1_HRS_NULL_INDEX, or something similar, be better?

> heapRegionSeq.hpp
>  - Could you move the class comment to above the class declaration?

Done.

>  - type on line 147: early of doHeapRegion -> early if doHeapRegion

Good catch. Fixed.

Updated webrev here:

http://cr.openjdk.java.net/~tonyp/7045330.7042285/webrev.1/

Tony

> 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
>>
>