RFR (S): 8073632: Make auxiliary data structures know their own translation factor

[Thomas Schatzl]

Hi Stefan,

On Wed, 2015-04-22 at 13:25 +0200, Stefan Karlsson wrote:
> 
> On 2015-04-22 11:02, Thomas Schatzl wrote:
> > Hi Stefan,
> >
> > On Wed, 2015-04-22 at 10:21 +0200, Stefan Karlsson wrote:
> >> Hi Thomas,
> >>
> >> On 2015-04-22 09:56, Thomas Schatzl wrote:
> >>>>     can I have reviews for this change that cleans up recent changes to
> >>>> memory management in G1. In particular, for every class that represents
> >>>> an auxiliary data structure in G1 it adds (and then uses) a specific
> >>>> function that returns the "heap mapping factor" (the amount of bytes a
> >>>> particular byte of that data structure corresponds in the java heap).
> >>>> This method is then used instead of gathering it from various places
> >>>> when allocating that data structure.
> >>>>
> >>>> CR:
> >>>> https://bugs.openjdk.java.net/browse/JDK-8073632
> >>>>
> >>>> Webrev:
> >>>> http://cr.openjdk.java.net/~tschatzl/8073632/webrev/
> >> http://cr.openjdk.java.net/~tschatzl/8073632/webrev/src/share/vm/gc_implementation/g1/g1SATBCardTableModRefBS.hpp.frames.html
> >>
> >> Why is this implemented as a call into G1BlockOffsetSharedArray?
> >>
> >>    157   // Returns how many bytes of the heap a single byte of the Card
> >> Table corresponds to.
> >>    158   static size_t heap_map_factor() {
> >>    159     return G1BlockOffsetSharedArray::heap_map_factor();
> >>    160   }
> > You are correct, that is unnecessary. It is better to use
> > CardTableModRefBS::card_size.
> >
> > Fixed in
> > http://cr.openjdk.java.net/~tschatzl/8073632/webrev.1
> > Diff webrev:
> > http://cr.openjdk.java.net/~tschatzl/8073632/webrev.0_to_1
> 
> That looks better.
> 
> It still seems weird that the card_count is set up using 
> G1BlockOffsetSharedArray:
> 
>     G1RegionToSpaceMapper* card_counts_storage =
>       create_aux_memory_mapper("Card counts table",
> G1BlockOffsetSharedArray::compute_size(g1_rs.size() / HeapWordSize),
> -                             G1BlockOffsetSharedArray::N_bytes);
> + G1BlockOffsetSharedArray::heap_map_factor());
> 
> I think the "Card counts table" needs its own functions as well.

For abstraction purposes, I agree. Internally these functions will just
forward to the card table one. The Card table(, BOT) and the card counts
table were intentionally made to have the same granularity, so I think
this is reasonable to reuse its methods internally.

Fixed in
http://cr.openjdk.java.net/~tschatzl/8073632/webrev.2
Diff webrev:
http://cr.openjdk.java.net/~tschatzl/8073632/webrev.1_to_2

I put the implementations in the cpp file because I do not see a
performance problem for them.

Thanks,
  Thomas