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

[Bengt Rutisson]

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...



g1BiasedArray.hpp

  protected:
   // the real base address
   address _base;
   // the length of the array
   size_t _length;
   // base address biased by "bias" elements
   address _biased_base;
   // the bias, i.e. the offset biased_base is located to the right in 
elements
   size_t _bias;
   // the amount of bits to shift right when mapping to an index of the 
array.
   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.


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 
#ifndefs.

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 
get_from_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?

Thanks,
Bengt



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:
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-February/005937.html
>
> 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.
>
> bugs.sun.com
> http://bugs.sun.com/view_bug.do?bug_id=7163191
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/7163191/webrev/
>
> Testing:
> jprt, dacapo benchmarks
>
> Thanks,
>    Thomas
>