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

[Gerard Ziemski]

Thank you Kim for the review!

> On Dec 4, 2018, at 12:36 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> 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;

Fixed.


> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/symbolTable.cpp
> 791   size_t get_dead_count() {
> 
> [pre-existing] Function should be const.

Fixed. Also fixed get_load_factor(), get_dead_factor() and get_re_sym(), though technically we should mark most of the functions this way, but fixing the getters is a good start.


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

add_items_count_to_clean() -> set_item_clean_count()

as part of the original work for JDK-8195100. Would you like to suggest a different name?


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

Thank you.

Issue:   https://bugs.openjdk.java.net/browse/JDK-8209387
Webrev:  http://cr.openjdk.java.net/~gziemski/8209387_rev2
Testing: Mach5 hs_tier1,2,3,4,5 in progress…


cheers