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

[coleen.phillimore at oracle.com]

One small comment below.

On 12/4/18 12:39 PM, Gerard Ziemski wrote:
> 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?

So symbolTable.cpp has set_item_clean_count() and StringTable has the 
equivalent add_items_count_to_clean().

Ah, but they're different.  I think the name change Kim wanted was in 
StringTable: add_items_to_clean_count -> add_items_to_clean without the 
count in the name.  Can you make this small change too?

Thanks,
Coleen

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