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

Thomas Schatzl thomas.schatzl at oracle.com
Mon Apr 14 09:08:23 UTC 2014


Hi Bengt,

On Mon, 2014-04-14 at 10:13 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> Looks good. Nice work!
> 
> A couple of minor nits:
> 
> Both _cur_region_cur_cards and HeapRegion::CardsPerRegion are size_t, so 
> there are a few unnecessary casts in the code I think:
> 
> 1058   _cur_region_cur_card((size_t)HeapRegion::CardsPerRegion),
> 
> 1095   if (_cur_region_cur_card == (size_t)HeapRegion::CardsPerRegion) {
> 
> I think the message for this assert is a bit strange.
> 
> 1107   guarantee(_cur_region_cur_card < HeapRegion::CardsPerRegion, 
> "card index must be within heap");
> 
> We are not verifying that the card index is within the heap. It is 
> verifying that the current card is within a heap region.

All fixed.

> Actually this raises another question. The name _cur_region_cur_card 
> lead me to believe that the region was somehow encoded in this value, 
> but it seems to only be the current card index (for the region that we 
> have stored in _finc_cur_prt). How about changing the name to just 
> _cur_card? Not sure it is better. I just got confused by the name, but I 
> do realize that the intent is that it is the current card index for the 
> regions we are iterating over.

I renamed it to _cur_card_in_prt. Note that a PRT corresponds to a
single region at this time, so the original does not sound too far
fetched.

> The method switch_to_next_prt() actually don't step to the next or 
> verify that it is switching to the next PRT. Can we rename it to just 
> switch_to_prt() ?

Done.

Webrev:
http://cr.openjdk.java.net/~tschatzl/8037344/webrev.3/

Incremental webrev:
http://cr.openjdk.java.net/~tschatzl/8037344/webrev.3.diff/

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list