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

[Bengt Rutisson]

Tony,

I finally got around to doing this review. Overall it looks good, thanks 
for cleaning this up.

A couple of comments:

- reuse_if_possible(HeapWord* bottom)
   * bottom is only used in an assert - hardly worth passing it as a 
parameter.
   * I thnink the name is difficult to understand. I would prefer to 
wrap this code in a method called get_heap_region() or something like that:

       HeapRegion* hr = _hrs.reuse_if_possible(base);
       if (hr == NULL) {
         // Create a new HeapRegion.
         MemRegion mr(base, high);
         bool is_zeroed = !_g1_max_committed.contains(base);
         hr = new HeapRegion(_bot_shared, mr, is_zeroed);

         // Add it to the HeapRegionSeq.
         _hrs.insert(hr);
       } else {
         assert(hr->bottom() == base, "post-condition");
       }

- _regions_biased
   * If this is an optimization that is important I think we should use 
it for addr_to_region() as well. I think this could be done by keeping a 
biased_length and not just a length.
   * if this optimization is not important I think we should avoid 
complicating the code with it.

- G1_INVALID_HRS_INDEX
   * I agree with Stefan that it is a bit strange that this is defined 
in heapregion.hpp. I'll look at the following reviews and get back to 
you on this.
   * I would also prefer to have it as a "static const size_t 
G1_INVALID_HRS_INDEX = (size_t) -1" rather than a #define. It is more 
C++ like and that way it is possible to see the value in a debugger.

Bengt

On 2011-06-02 00:15, Tony Printezis wrote:
> 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
>>>
>>