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

[coleen.phillimore at oracle.com]

Looks good!
Coleen

On 12/6/18 11:15 AM, Gerard Ziemski wrote:
>> On Dec 5, 2018, at 7:03 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>>> On Dec 5, 2018, at 9:26 AM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
>>>
>>>
>>>> On Dec 4, 2018, at 6:20 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>>
>>>>> On Dec 4, 2018, at 12:39 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
>>>>>
>>>>> Thank you Kim for the review!
>>>>>
>>>>>> On Dec 4, 2018, at 12:36 AM, Kim Barrett <kim.barrett at oracle.com> wrote: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?
>>>> The name you describe is in SymbolTable.
>>>>
>>>> Also as part of JDK-8195100, StringTable::add_items_to_clean() was changed to
>>>> StringTable::add_items_count_to_clean().  That is the name change that I objected to at
>>>> the time, and which is referred to by this item in the bug report.  Please revert it back to
>>>> the original name.
>>> I think that we should try to keep the names same in both String and Symbol tables. May I suggest that we either rename:
>>>
>>> #1 StringTable: add_items_count_to_clean() -> set_item_clean_count()
>>>
>>> #2 StringTable: add_items_count_to_clean() -> add_items_to_clean()
>>>   SymbolTable:     set_item_clean_count() -> add_items_to_clean()
>>>
>>> I personally prefer #1 as we don’t actually add “items” themselves, but only their “count”.
>> They do different things, so it is unsurprising that they have different names.
>> For StringTable, we’re accumulating a total from multiple threads.  “set_” prefix would be wrong here.
>> For SymbolTable, we’re setting value from one thread’s work.
>>
>> These are each part of a little “suite” of helper functions dealing with the number of items.
>> I don’t love the existing names, but they are good enough.  But this one unmotivated name
>> change stood out as inconsistent when I was doing the earlier review, so I requested that
>> name change be backed out.  And that's still all I want.
> In the end they do the exactly the same thing, i.e. update the dead counter that is used to calculate dead load factor, so that we know when to clean.
>
> String table accumulates (which I think might be wrong, but need to think more about it), while Symbol table sets, so yeah that’s different, but the name “add_items_to_clean()” implies that we're adding “items” themselves, so personally I prefer “add_items_count_to_clean()”, but this discussion has gotten far enough. I reverted the String table API to what it used to be.
>
> Issue:   https://bugs.openjdk.java.net/browse/JDK-8209387
> Webrev:  http://cr.openjdk.java.net/~gziemski/8209387_rev3
> Testing: Mach5 hs_tier1,2,3,4,5 in progress…
>
>
> cheers
>
>