RFR(M): 7163191: G1: introduce a "heap spanning table" abstraction

[Bengt Rutisson]

Hi John,

Sorry for being really, really, really late with the response to this 
review.

I think the late response has caused some problems too. Your patch 
changes HeapRegionSeq::shrink_by() which I recently refactored. We 
should really refactor HeapRegionSeq::expand_by() in a similar way. But 
this means that your patch needs to be updated to work with the new 
versions. I think it should fit pretty well since the new implementation 
is based on region counts instead of byte sizes.

After looking at this again, I am thinking that it might be good to 
re-think the design a bit. I am not sure that it is a good design to 
have HeapRegionSeq inherit from G1HeapSpanningTable. I think the code 
might be simpler if we used aggregation instead an let HeapRegionSeq 
have an instance of a G1HeapSpanningTable. I also don't think the 
_high_water_mark logic should be part of the spanning table. It seems 
very specific to the HeapRegionSeq class. (In fact, I am not sure it is 
a good idea to have this logic at all. We can potentially waste a lot of 
memory if the heap grows and then shrinks a lot. But that can be a 
separate discussion.)

Some other comments:

Why does create_new_array_base() do:

     T* array = (T*) NEW_C_HEAP_ARRAY(char, array_size_bytes(), mtGC)

Instead of:

     T* array = (T*) NEW_C_HEAP_ARRAY(T, (size_t) _max_length, mtGC)

With the second approach we can remove the method array_size_bytes().

In HeapRegionSeq::expand_to() we do:
       hr = g1h->new_heap_region(index, next_hr_bottom);
       // If allocation fails, bail out and return what we have done so far.
       if (hr == NULL) break;

new_heap_region uses the global operator new, which will exit the VM if 
it fails. So, I don't thnk we need th hr == NULL check.

The change in HeapRegionSeq::free_suffix() seems strange.

Sorry again for the late comments.
Bengt



On 2/26/13 7:20 PM, John Cuthbertson wrote:
> Hi Everyone,
>
> A new version of this that incorporates some feedback from Bengt and 
> Thomas can be found at: 
> http://cr.openjdk.java.net/~johnc/7163191/webrev.1/
>
> Summary of Changes
> * Removed the split between the templated base class. The routines 
> that were in the .cpp have been moved into the .hpp.
> * default value is now a virtual function that subclasses have to 
> implement.
> * The new .cpp has been removed.
> * Minor cleanups suggested by Bengt.
> * Minor cleanups suggested by Thomas Schatzl.
>
> Testing:
> GC test suite.
> I've also updated and tested the patch that converts the two fast 
> in-cset arrays to be sub-classes of the spanning table. This will be 
> sent out for review once this change is pushed.
>
> Thanks,
>
> JohnC
>
> On 2/14/2013 2:16 PM, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> Here's another change from Tony that I've been maintaining since he 
>> left Oracle. Can I have a volunteer look over the changes? They look 
>> good to me. The webrev can be found at: 
>> http://cr.openjdk.java.net/~johnc/7163191/webrev.0/
>>
>> Summary:
>> Currently in G1 there are several tables where a each element is used 
>> to hold some information about a fixed size subdivision of the heap. 
>> For example the HeapRegion table and the fast CSet test tables. This 
>> change encapsulates the base properties of these tables in a generic 
>> heap spanning table and uses the abstraction for the heap region 
>> sequence table. A future change will extend this to the fast cset 
>> test arrays.
>>
>> Testing consisted of the GC test suite with a low marking threshold 
>> and heap verification. Testing did catch an assertion failure that I 
>> have included a small fix for. As with the additional marking 
>> validation changes, if no-one objects, I will push this change with 
>> Tony as the author and myself as a reviewer.
>>
>> JohnC
>