RFR (XL): 8151386: Extract card live data out of G1ConcurrentMark

[Thomas Schatzl]

Hi,

On Tue, 2016-03-29 at 15:06 +0200, Mikael Gerdin wrote:
> Hi Thomas,
> 
> I looked at webrev.2 but I'll reply here since you cut out the email 
> context.
> 
> On 2016-03-23 16:39, Thomas Schatzl wrote:
> > Hi Mikael,
> > 
> >    thanks for your review.
> > 
> > On Wed, 2016-03-23 at 13:56 +0100, Mikael Gerdin wrote:
> > > Hi Thomas,
> > > 
> > > On 2016-03-18 14:36, Thomas Schatzl wrote:
> > > > Hi all,
> > > > [...]
> > > > 
> > > > http://cr.openjdk.java.net/~tschatzl/8151386/webrev.0a/ contain
> > > > s
> > > > the
> > > > "move" with adaptations to make it work. (Note that I know that
> > > > this
> > > > change alone leaks memory with -XX:+VerifyDuringGC as the
> > > > temporary
> > > > bitmaps used there are not freed)
> > > > 
> > > > http://cr.openjdk.java.net/~tschatzl/8151386/webrev.0b/ is the
> > > > change
> > > > that tries to improve the code quality and fixes the mentioned
> > > > issue.
> > > > 
> > > > http://cr.openjdk.java.net/~tschatzl/8151386/webrev.0/ contains
> > > > the
> > > > full change.
> > > 
> > > in g1CardLiveData.cpp
> > > 
> > > G1CardLiveData::initialize
> > > I don't think card live data should recompute the region size.
> > > Ff you are feeling paranoid about the heap region size then
> > > keep some asserts and use HeapRegion::GrainBytes (it's set up ac
> > > the
> > > construction of G1CollectorPolicy, which is before
> > > G1CollectedHeap is
> > > created)
> > 
> > For unit-testing the remembered set later (scrubbing tests) I need
> > to
> > be completely independent of global variables with this data
> > structure.
> > 
> > Ideally there would be a global "G1HeapSettings" class that
> > collects
> > these that is used everywhere and could be referenced, but this is
> > not
> > the case right now, and is a huge change.
> > 
> > Even if that were not a problem, for testing it is less resource
> > intensive to create smaller than usually available heaps/regions.
> > 
> > I can change this now, but will need to add something like this
> > soon
> > anyway. If you want I can remove it for now, but I kind of not see
> > the
> > point.
> 
> I see, let's leave your change as is for now then.
> 
> > 
> > >     71   _cards_per_region = region_size >>
> > > G1SATBCardTableModRefBS::card_shift;
> > > Can you make this a division instead? There's no extra cost and
> > > it
> > > makes the code more obviously correct.
> > 
> > Done :)
> 
> No :)
> 
> http://cr.openjdk.java.net/~tschatzl/8151386/webrev.2/src/share/vm/gc
> /g1/g1CardLiveData.cpp.html
> 
> 71 _cards_per_region = region_size >>
> G1SATBCardTableModRefBS::card_shift;
> 72
> 
> Also, you are a bit inconsistent in using the
> G1CardLiveData::bm_word_t 
> typedef, if you want to keep it you should probably try to use it in 
> more places, otherwise you should just remove it IMO.

Now.

http://cr.openjdk.java.net/~tschatzl/8151386/webrev.3/ (full)
http://cropenjdk.java.net/~tschatzl/8151386/webrev.2_to_3 (diff)

Btw, in the meantime I did another jprt run with the changes (not
including these particular ones).

Thomas