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

[sangheon.kim at oracle.com]

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,
>>>
>>>    may I have reviews for this change that ultimately makes sure 
>>> that the number of occupied cards in a remembered set is only 
>>> growing by providing a per-OtherRegionsTable count that is 
>>> atomically updated when adding a remembered set entry.
>>>
>>> Note that this count may not be completely accurate due to races 
>>> when deleting a PerRegionTable (which is a known issue) from an 
>>> OtherRegionsTable; but that is no different than before.
>>>
>>> This helps improving the predictions in the young gen remset 
>>> sampling thread, and increase the performance of getting the 
>>> occupancy count.
>>>
>>> Based on JDK-8233997, and JDK-8233998 also out for review.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8233919
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8233919/webrev/
>> I like this change and it looks good in general, just one small comment:
>> src/hotspot/share/gc/g1/heapRegionRemSet.cpp
>> ---
>>   247   bool added = prt->add_reference(from);
>>   248   Atomic::add(num_added_by_coarsening + (added ? 1 : 0), 
>> &_num_occupied, memory_order_relaxed);
>>
>> 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.

Thanks,
Sangheon


>
> Thanks for your review,
>   Thomas
>
>