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

[Mikael Gerdin]

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 :)


+  bool should_expand() const { return _occupied_entries == _num_entries; }

Personally I prefer to keep this a ">" instead of having an exact check.

Otherwise the change looks good.
/Mikael


>
> Testing:
> jprt, gcbasher with VerifyRememberedSets
>
> Thanks,
>    Thomas
>