RFR (XS): 8155721: Sparse remset wastes half of entry memory

[Mikael Gerdin]

On 2016-05-03 11:28, Thomas Schatzl wrote:
> Hi Mikael,
>
> On Mon, 2016-05-02 at 16:55 +0200, Mikael Gerdin wrote:
>> Hi Thomas,
>>
>> On 2016-05-02 16:03, Thomas Schatzl wrote:
>>>
>>> Hi Mikael,
>>>
>>> On Mon, 2016-05-02 at 13:05 +0200, Mikael Gerdin wrote:
>>>>
>>>> Hi Thomas,
>>>>
>>>> On 2016-05-02 12:17, Thomas Schatzl wrote:
>>>>>
>>>>>
>>>>> Hi all,
>>>>>
>>>>>      can I have reviews for this pretty small change that
>>>>> (basically)
>>>>> halves memory usage of the sparse prt?
>>>>>
>>>>> Looking through the code showed that G1 allocates
>>>>> capacity SparsePRTEntrys (the container for the cards for a
>>>>> particular
>>>>> region), but already expands the hash table at capacity/2+1. So
>>>>> almost
>>>>> half of these SparsePRTEntrys are never used and need not be
>>>>> allocated/cleared/etc.
>>>>>
>>>>> This also uncovered a off-by-one bug in
>>>>> RSHashTable::alloc_entry()
>>>>> that
>>>>> was benign before but caused crashes when allocating just what
>>>>> is
>>>>> required.
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8155721
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~tschatzl/8155721/webrev/
>>>> Good catch,
>>>> however
>>>>
>>>> I'm not sure I'm fond of keeping the inverse of the occupancy
>>>> factor.
>>>> I think the code would be a bit clearer if we made it a floating
>>>> point
>>>> number.
>>>>
>>>> Also, I think it should be CapitalizedCamelCase since it's a
>>>> constant.
>>> I made it a floating point number, but in return precalculate the
>>> num_entries value to avoid any issues due to rounding etc.
>>>
>>> http://cr.openjdk.java.net/~tschatzl/8155721/webrev.0_to_1 (diff)
>>> http://cr.openjdk.java.net/~tschatzl/8155721/webrev.1
>> If you want to make this change neutral WRT the size of RSHashTable
>> you can remove _capacity_mask which appears to always be (_capacity -
>> 1) and always accessed through a getter function :)
>
> I think that is not worth the effort. There is only one RSHashTable per
> region remset anyway.

Ok.

>> +  bool should_expand() const { return _occupied_entries ==
>> _num_entries; }
>>
>> Personally I prefer to keep this a ">" instead of having an exact
>> check.
>
> I would like to keep this, while the more imprecise >= would be as
> fine, I am then always scratching my head why this is the case when
> looking at it.
>
> The sparse prt is only ever added to under lock.
>
> So I would prefer to keep it this way unless you are really opposed to
> that.

Ok.

/Mikael

>
> Thanks,
>    Thomas
>