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

[Stefan Johansson]

Looks good,
StefanJ

On 2019-11-29 10:25, Thomas Schatzl wrote:
> 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
>>
>