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

[Bengt Rutisson]

Hi Thomas,

Thanks for fixing all of the minor things.

New webrev looks good!

Bengt


On 2014-04-14 11:08, Thomas Schatzl wrote:
> 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
>