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

[Thomas Schatzl]

Hi,

On Fri, 2013-09-13 at 14:11 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> Sorry for being so late with the feedback on this.
> 
> I have some comments and questions:
> 
> excludeSrc.make
> Why do you need to exclude g1BiasedArray.hpp? Seems strange to have to 
> exclude an hpp file...

Leftover from a time when there have been separate hpp/cpp files. The
tests added the cpp file again anyway though :)

> g1BiasedArray.hpp
> 
>   protected:
>    // the real base address
>    address _base;
[...]
>    uint _shift_by;
> 
> Would prefer to have the comments on the same line. Not sure that _base 
> and _length really need comments.

Done.

> I think I would prefer to inline initialize_base() inside initialize(). 
> I think it is nicer to have all the initialization in one place. Best 
> would have been if we didn't have to have the initialize methods at all 
> and let the constructor do all the initialization. But I realize that 
> such a change propagate up the call stack to HeapRegionSeq and 
> G1CollectedHeap, so it is difficult to do. If you think the initialize() 
> method gets too large I would suggest factoring out the asserts to some 
> kind of verification method instead of splitting the actual 
> initialization up.

I prefer to have the initialize_base() and initialize() separate:
initialize_base and initialize do initialization on a different "level":
initialize works on heap basis, while initialize is mostly what would
otherwise be part of the constructor.

I am thinking if you are not specifically mapping HeapWords to the array in
some descendant.

> What was the rational for having G1BiasedMappedArrayBase? I remember us 
> talking about it, but I can't remember the details now.

G1BiasedMappedArrayBase collects the methods that are independent of any
specialization, saving the code duplication when instantiating.

> I think the #ifndef PRODUCT statements are polluting the code a bit. 
> Would you mind changing the verify_*-methods to be declared 
> PRODUCT_RETURN instead? That way the call sites don't have to have any 
> #ifndefs.

Done.

> The methods G1BiasedMappedArray::set_mapped() and 
> G1BiasedMappedArray::mapped_to() are not used. Can we remove them?
> 
> The methods G1BiasedMappedArray::base() and 
> G1BiasedMappedArray::biased_base() could be made private.

Both are interesting for descendants, e.g. fast iteration over parts of
the array. Set_mapped() is the inverse of get_mapped(), useful if you
only have a HeapWord* and want to avoid the index/base calculation.

> About the naming. I get a little confused about get() and get_mapped(). 
> What about naming both more specific. Maybe get_by_index() and 
> get_from_address()?

Done. Used get/set_by_index() and renamed get/set_mapped() to
get/set_by_address().

> Also, it seems like G1BiasedMappedArray is a good candidate for 
> InternalVMTests. Would you like to make an attempt to write a test for it?

Done. Note that as the code needs a backing heap to do actual get/set
methods, these tests are limited to address calculation/boundary tests.

Found one issue where the verification has been too strict: in particular, it
should be allowed to get the address of the array element one past the array.

:)

New webrev:
http://cr.openjdk.java.net/~tschatzl/7163191/webrev.1/

Testing:
jprt, internal test cases

Hth,
Thomas