RFR (M): 7163191 G1: introduce a "heap spanning table" abstraction
thomas.schatzl at oracle.com
Mon Sep 16 18:52:00 UTC 2013
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
> 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.
jprt, internal test cases
More information about the hotspot-gc-dev