RFR: 8242038: G1: Lazily initialize RSHashTables

Kim Barrett kim.barrett at oracle.com
Wed Apr 8 04:06:18 UTC 2020


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

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

------------------------------------------------------------------------------





More information about the hotspot-gc-dev mailing list