RFR 8209387: Follow ups to JDK-8195100 Use a low latency hashtable for SymbolTable

[Kim Barrett]

> On Dec 1, 2018, at 12:27 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
> 
> Hi all,
> 
> Please review these couple small “cleanup” tasks as a followup to JDK-8195100 (Use a low latency hashtable for SymbolTable)
> 
> The first 2 of these affect both Symbol and String hashtables:
> 
> #1 We replace macros with constant constructs.
> 
> #2 We use “size_t" for counters, instead of “int”, to reduce the need of castings.
> 
> #3 I expanded on the comment for “mark_item_clean_count"
> 
> Please notice that:
> 
> A) “_alt_hash” is handled separately by https://bugs.openjdk.java.net/browse/JDK-8214449
> 
> B) I don’t personally agree with the 2nd point raised in this issue (i.e. placement of "assert that sym->refcount() == 0” in “delete_symbol”), so I’d like to leave the code as is.
> 
> Issue:   https://bugs.openjdk.java.net/browse/JDK-8209387
> Webrev:  http://cr.openjdk.java.net/~gziemski/8209387_rev1
> Testing: Mach5 hs_tier1,2,3,4,5
> 
> 
> Cheers

------------------------------------------------------------------------------
src/hotspot/share/classfile/stringTable.cpp
  57 static const double PREF_AVG_LIST_LEN = 2.0;
  59 static const size_t END_SIZE = 24;
  61 static const size_t REHASH_LEN = 32;
  63 static const double CLEAN_DEAD_HIGH_WATER_MARK = 0.5;

"static" isn't needed for any of these; (non-volatile, non-extern)
const has internal linkage.

Similarly here:
src/hotspot/share/classfile/symbolTable.cpp
  45 static const double PREF_AVG_LIST_LEN = 8.0;
  49 static const size_t END_SIZE = 17;
  51 static const size_t REHASH_LEN = 100;
  55 static const double CLEAN_DEAD_HIGH_WATER_MARK = 0.0;
  57 static const size_t ON_STACK_BUFFER_LENGTH = 128;

------------------------------------------------------------------------------
src/hotspot/share/classfile/symbolTable.cpp
 791   size_t get_dead_count() {

[pre-existing] Function should be const.

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

What happened to this entry in the CR:

src/hotspot/share/classfile/stringTable.hpp
87 size_t add_items_count_to_clean(size_t ndead);

This name change doesn't seem right.

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

I continue to disagree about the assertion in free_node vs
delete_symbol, and think the reasoning being given for its present
location is wrong. If freeing the node's memory actually required the
refcount to be zero (as claimed in the CR and in Coleen's review),
then the code would be broken, because the refcount might instead be
PERM_REFCOUNT. Fortunately, that claim is false.

The real precondition on freeing the node's memory is that the symbol
can be deleted. By putting it after delete_symbol, we know we've met
that precondition, since we must have met delete_symbol's preconditions
and its postconditions must hold.

In delete_symbol, we handle the PERM_REFCOUNT one way. Otherwise, we
delete the symbol, which has the refcount being zero as a precondition.
That precondition check should be near the code that cares.

(BTW, I think it's a bug that the deleted symbol is passed to
BaseConfig::free_node, but that's an entirely separate issue.
Fortunately, the deleted object is not used for anything there.)

That said, I'm not going to veto over the details of this assert.

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