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