RFR (M): 8233919: Incrementally calculate the occupied cards in a heap region remembered set

[Thomas Schatzl]

Hi all,

   thanks for your reviews - however I unfortunately found a small bug 
in the current implementation that I would like to have fixed.

In particular, when adding sparse cards the code incremented 
num_occupied always when SparsePRT::add_card returns true. However it 
also returns true if the card is duplicate, i.e. has already been found, 
which means that we should not increase the count.

The actual difference is rather small as most of the duplicates are 
filtered out by the FromCardCache, but it occurs.

The following patch fixes this. The change looks big, but is basically 
reshuffling code to accomodate the use of SparsePRTEntry::AddCardResult 
in SparsePRT::add_card.

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

Thanks, and sorry for the inconvenience,
   Thomas

On 26.11.19 16:41, sangheon.kim at oracle.com wrote:
> Hi Thomas,
> 
> On 11/26/19 1:04 AM, Thomas Schatzl wrote:
>> Hi Sangheon,
>>
>>   thanks for looking at this.
>>
>> On 25.11.19 22:22, sangheon.kim at oracle.com wrote:
>>> Hi Thomas,
>>>
>>> On 11/21/19 2:41 AM, Thomas Schatzl wrote:
>>>> Hi,
>>>>
>>>> On 20.11.19 11:42, Stefan Johansson wrote:
>>>>> Hi Thomas,
>>>>>
>>>>> On 2019-11-12 16:24, Thomas Schatzl wrote:
>>>>>> Hi all,
>> [...]>>>
>>>>> I would prefer:
>>>>> if (prt->add_reference(from)) {
>>>>>    num_added_by_coarsening++;
>>>>> }
>>>>> Atomic::add...
>>>>>
>>>>> I you disagree, leave it as is.
>>>>
>>>> Fixed in
>>>> http://cr.openjdk.java.net/~tschatzl/8233919/webrev.0_to_1/
>>>> http://cr.openjdk.java.net/~tschatzl/8233919/webrev.1/
>>> Webrev.1 looks good in general.
>>>
>>> ========================
>>> g1CollectionSet.cpp
>>>   250   assert(old_rs_length <= new_rs_length,
>>>   251          "Remembered set sizes must increase (changed from " 
>>> SIZE_FORMAT " to " SIZE_FORMAT " region %u type %s)",
>>>   252          old_rs_length, new_rs_length, hr->hrm_index(), 
>>> hr->get_short_type_str());
>>>   - I feel 'must increase' like 'old_rs_length < new_rs_length'. If 
>>> you don't agree leave it as is. :)
>>>
>>> ========================
>>> heapRegionRemSet.cpp
>>>   200         Atomic::inc(&_num_occupied, memory_order_relaxed);
>>> - I already asked to Thomas offline. He said Atomic operation is not 
>>> necessary in this version but it is necessary for future patch when 
>>> the lock is removed.
>>
>> http://cr.openjdk.java.net/~tschatzl/8233919/webrev.1_to_2/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8233919/webrev.2/ (full)
>>
>> Fixes the comment by changing the comment text to:
>>
>>  251          "Remembered set decreased (changed from " SIZE_FORMAT " 
>> to " SIZE_FORMAT " region %u type %s)",
>>
> Looks good.
> 
> Thanks,
> Sangheon
> 
> 
>> Thanks,
>>   Thomas
>