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

[Bengt Rutisson]

Hi Thomas,

Sorry for taking so long to get back to you about this.

Looks good.

Thanks for adding the test! One minor nit about the test:

Lately there has been some unit tests added that use a wrapper function 
like test_g1biasedarray() that you introduce. These tests, for example 
TestReservedSpace_test(), use a forward declaration in jni.cpp rather 
than including the hpp file and they also use a naming convention that 
kind of mimics a static method on the class to be tested. This is pretty 
new, so I don't know if this is the accepted way to do it, but unless 
you have strong feelings I suggest following that same pattern.

Bengt

On 9/16/13 8:52 PM, Thomas Schatzl wrote:
> 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
>
>