RFR (M): 7163191 G1: introduce a "heap spanning table" abstraction
bengt.rutisson at oracle.com
Tue Sep 24 02:45:26 PDT 2013
Sorry for taking so long to get back to you about this.
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.
On 9/16/13 8:52 PM, Thomas Schatzl wrote:
> 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:
>> 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 :)
>> // 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.
>> 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
>> 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
> Done. Used get/set_by_index() and renamed get/set_mapped() to
>> 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:
> jprt, internal test cases
More information about the hotspot-gc-dev