RFR: 8242038: G1: Lazily initialize RSHashTables

[Thomas Schatzl]

Hi,

On 08.04.20 06:06, Kim Barrett wrote:
>> On Apr 7, 2020, at 10:52 AM, Claes Redestad <claes.redestad at oracle.com> wrote:
>>
>>
>>
>> On 2020-04-07 10:03, Kim Barrett wrote:
>>>> On Apr 7, 2020, at 3:57 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>> Morover, the RSHashTable constructor calls RSHashTable::clear(). That
>>>> calls SparsePRTEntry::size(), whose value is computed from
>>>> G1RSetSparseRegionEntries, which is a product flag which doesn't have
>>>> its final value at static initialization time. Oops!
>>> Oh, and SparsePRTEntry::size() is also called to size the array.
>>> Probably the thing that makes this not just crash quickly is that the special
>>> empty_table doesn’t seem to get used as anything other than a marker / sentinal.
>>
>> 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.

> 
> ------------------------------------------------------------------------------
> 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.

Thanks,
   Thomas