RFR (M): 7163191 G1: introduce a "heap spanning table" abstraction
bengt.rutisson at oracle.com
Fri Sep 13 05:11:41 PDT 2013
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...
// the real base address
// the length of the array
// base address biased by "bias" elements
// the bias, i.e. the offset biased_base is located to the right in
// the amount of bits to shift right when mapping to an index of the
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
What was the rational for having G1BiasedMappedArrayBase? I remember us
talking about it, but I can't remember the details now.
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.
About the naming. I get a little confused about get() and get_mapped().
What about naming both more specific. Maybe get_by_index() and
Also, it seems like G1BiasedMappedArray is a good candidate for
InternalVMTests. Would you like to make an attempt to write a test for it?
On 8/29/13 10:08 PM, Thomas Schatzl wrote:
> Hi all,
> can I have reviews for the following change?
> It implements a generic class that allows mapping of subdivision of the
> Java heap to single elements of an array. It provides a biased pointer
> for fast access and some convenience functions.
> Possible applications are mappings from heap addresses to HeapRegion
> instances (the heap spanning table), the "fast CSet" tables to detect
> whether a region is in the collection set and others.
> This change implements the heap spanning table using this new "biased
> array" instead of manual management/setup of the array.
> John Cuthbertson already had a stab at it, polishing a change from Tony
> Printezis. This change is mostly a rewrite, as internal discussion found
> that moving some functionality of HeapRegionSeq, the watermark, into
> that generic "biased array" was undesirable.
> The original mail thread starts here:
> The main change is in the new file G1BiasedArray.hpp, providing the new
> functionality. Everything else is use of that new data structure (an
> instance of G1BiasedMappedArray<HeapRegion*>) in the HeapRegionSeq
> class, and resulting cleanup.
> jprt, dacapo benchmarks
More information about the hotspot-gc-dev