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

[Bengt Rutisson]

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.

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.

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() ?

Thanks,
Bengt





On 2014-04-09 15:05, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2014-04-09 at 14:37 +0200, Mikael Gerdin wrote:
>> 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.
> Done in http://cr.openjdk.java.net/~tschatzl/8037344/webrev.2/
>
> void HeapRegionRemSetIterator::switch_to_next_prt(PerRegionTable* prt) {
>     assert(prt != NULL, "Cannot switch to NULL prt");
>     _fine_cur_prt = prt;
>
>     HeapWord* r_bot = _fine_cur_prt->hr()->bottom();
>     _cur_region_card_offset = _bosa->index_for(r_bot);
>
> +  // The bitmap scan for the PRT always scans from _cur_region_cur_card + 1.
> +  // To avoid special-casing this start case, and not miss the first bitmap
> +  // entry, initialize _cur_region_cur_card with -1 instead of 0.
>     _cur_region_cur_card = (size_t)-1;
>   }
>
> Thanks,
> Thomas
>