RFR: 8242038: G1: Lazily initialize RSHashTables

[Claes Redestad]

On 2020-04-08 13:26, Thomas Schatzl wrote:
>>> Good catches, Kim,
>>>
>>> I've reworked the initialization of the empty RSHashTable
>>> to avoid calling SparsePRTEntry::size and doing C-heap allocation with a
>>> dedicated (private) constructor:
> 
> Thanks!
> 
>>>
>>> http://cr.openjdk.java.net/~redestad/8242038/open.01/
>>
>> ------------------------------------------------------------------------------ 
>>
>>   177   RSHashTable();
>>   178 public:
>>
>> Mostly in HotSpot I think we have a blank line between code and a
>> following access-specifier.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/gc/g1/sparsePRT.cpp
>>    90 static int empty_buckets[1];
>> ...
>>   103   _buckets[0] = NullEntry;
>>
>> How about
>>
>>    90 static int empty_buckets[] = { RSHashTable::NullEntry };
>>
>> and remove line 103.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/gc/g1/sparsePRT.cpp
>>   122     assert(_entries != NULL, "INV");
>>
>> I'd prefer "invariant" be spelled out.  I see there are a couple of
>> other nearby uses of the same abbreviation, but elsewhere in the file
>> it is spelled out.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/gc/g1/sparsePRT.cpp
>>   262 SparsePRT::~SparsePRT() {
>>   263   delete _table;
>>   264 }
>>
>> This needs to be updated to not delete when _table == &empty_table.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/gc/g1/sparsePRT.cpp
>>   300   if (last->capacity() < InitialCapacity) {
>>
>> I think clearer here would be to check if last == &empty_table.
>> Otherwise it's not obvious why we don't need to delete last.
> Agree with all above suggestions. Although in this case I'd tend to 
> invert the condition (i.e. != &empty_table), but there are no particular 
> measurements behind that.

Fixed.

> 
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/gc/g1/sparsePRT.hpp
>>    int _tbl_ind;         // [-1, 0.._rsht->_capacity)
>>
>> [pre-existing]
>>
>> I see no sign of -1 (or any other negative value) as a possibility for
>> this member. It is initialized to 0 in the constructor and is only
>> ever incremented. As such, I think it's type could be unsigned, and
>> the cast when comparing with _rsht->capacity() in RSHTBIter::has_next
>> would not be needed.
>>
>> ------------------------------------------------------------------------------ 
>>
> 
> The iteration completely changed in the changes how the card scanning is 
> done in jdk14, this is a leftover.

Ok, I've fixed the comment and changed type to uint (or do you prefer
"unsigned"):

http://cr.openjdk.java.net/~redestad/8242038/open.02/

Sanity-testing tier1+2 again.

/Claes