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

[Tony Printezis]

Hi Bengt,

Thanks for the code review. Please see inline.

On 06/08/2011 04:10 AM, Bengt Rutisson wrote:
>
> Tony,
>
> I finally got around to doing this review. Overall it looks good, 
> thanks for cleaning this up.

Yeah, and sorry for the slightly lengthy change. I think the new version 
is much more understandable.

> A couple of comments:
>
> - reuse_if_possible(HeapWord* bottom)
>   * bottom is only used in an assert - hardly worth passing it as a 
> parameter.

I did a few different versions of that code which is how that parameter 
ended up there but ultimately not being "really" used. I have to say 
that, normally, passing a parameter for an assert in a non-critical part 
of the code would actually be acceptable (IMHO). Having said that, in 
this case the caller can do the assert after getting back the result so, 
you're right, I'll remove it.

>   * 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");
>       }

Yeah, you're right the name was not the best. How the following?

       HeapRegion* hr = _hrs.expand_to_next_region();
       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);
         _hrs.expand_to_next_region(hr);
       } else {
         assert(hr->bottom() == base, "post-condition");
       }

expand_to_next_region() is more descriptive. Also note that renamed 
insert(HR *hr) as expand_to_next_region(HR* hr) as the two are in a way 
similar (and opposite to shrink).

> - _regions_biased
>   * If this is an optimization that is important 

It was one of the things that was showing up during the profiling we did 
a few weeks ago.

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

I was going to write a lengthy paragraph about the differences in 
assumptions between addr_to_region() and addr_to_region_unsafe(). But 
while looking at it I think we can do a bit better than the current 
version given that, if addr <= heap_bottom, it can only be NULL. So, let 
me rework this code a bit and let me know what you think of the new version.

>   * if this optimization is not important I think we should avoid 
> complicating the code with it.

I agree, but it looked as if it gave a small but measurable improvement.

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

OK. BTW, would you prefer G1_NULL_HRS_INDEX instead of INVALID?

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

In HotSpot we generally use macros for constants. Maybe, I like the fact 
that they are guaranteed to be treated as constants by the C++ 
compilers. :-)

More on the next e-mail...

Tony

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