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

[Thomas Schatzl]

Hi Stefan,

  thanks for the review.

Thanks,
  Thomas

On Thu, 2015-04-23 at 12:00 +0200, Stefan Karlsson wrote:
> On 2015-04-23 10:55, Thomas Schatzl wrote:
> > 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.
> 
> Looks good.
> 
> Thanks,
> StefanK
> 
> >
> > Thanks,
> >    Thomas
> >
> >
>