RFR (S): 8037344: Use the "next" field to iterate over fine remembered instead of using the hash table

[Mikael Gerdin]

On Wednesday 09 April 2014 14.34.06 Thomas Schatzl wrote:
> Hi,
> 
> On Wed, 2014-04-09 at 13:46 +0200, Mikael Gerdin wrote:
> > Hi Thomas,
> > 
> > On Wednesday 09 April 2014 12.02.45 Thomas Schatzl wrote:
> > > Hi all,
> > > 
> > >   I have been notified that the webrev does not apply to the jdk9/hs-gc
> > > 
> > > repository any more.
> > > 
> > > Here is an updated version:
> > > http://cr.openjdk.java.net/~tschatzl/8037344/webrev.1/
> > 
> > Just to make sure that I follow the subtleties in the code here, you set
> > _cur_region_cur_card to -1 here. Then when fine_has_next is called it
> > computes _cur_region_cur_card + 1 to get 0 and calls
> > _bm.get_next_one_offset?
> Exactly, this is to get the start condition right after switching from
> coarse to fine entries or to the next fine PRT.
> Get_next_one_offset() starts searching including the passed offset.
> 
> I will add a comment.

Thanks.

> 
> > 1115 void HeapRegionRemSetIterator::switch_to_next_prt(PerRegionTable*
> > prt) { 1116   assert(prt != NULL, "Cannot switch to NULL prt");
> > 1117   _fine_cur_prt = prt;
> > 1118
> > 1119   HeapWord* r_bot = _fine_cur_prt->hr()->bottom();
> > 1120   _cur_region_card_offset = _bosa->index_for(r_bot);
> > 1121
> > 1122   _cur_region_cur_card = (size_t)-1;
> > 1123 }
> > 
> > The rest of the code change looks ok, but this code is really complex and
> > I
> > hope that you've tested this with verification enabled to catch any tricky
> > bugs.
> 
> It took me a while to get it completely right to pass various benchmarks
> with verification. I do not exactly remember though what programs the
> change has been tested with - however this change is part of my stack of
> pending changes that is continuously used in developing new changes, so
> in the last month or so it got a lot of implicit testing.
> 
> This testing includes the usual benchmarks, and bigapps containing
> kitchensink 24h runs and such.

That's good. I think the only option for this code is to change it, test it 
and fix it if we break it.

> 
> Overall the change simplifies the old code significantly imo, however I
> agree that especially this iterator's interface complicates matters a
> lot.

Agreed.

/Mikael

> 
> Thanks,
>   Thomas