RFR(S) 8210875 Refactor CompactHashtable

[Calvin Cheung]

Hi Ioi,

Thanks for making the change and it looks good.

thanks,
Calvin

On 9/19/18, 2:46 PM, Ioi Lam wrote:
> Hi Calvin,
>
> Thanks for the review. I've added the #ifdefs as you suggested, and 
> tested that it worked with a minimal build (which excludes CDS).
>
> http://cr.openjdk.java.net/~iklam/jdk12/8210875-refactor-compact-hash.v03.delta/ 
>
>
> Thanks
>
> - Ioi
>
>
> On 09/19/2018 11:11 AM, Calvin Cheung wrote:
>> Hi Ioi,
>>
>> Just a quick question in compactHashTable.[c|h]pp, I’m wondering if 
>> the CompactHashtableWriter can be enclosed within #ifdef INCLUDE_CDS? 
>> Its usage in StringTable is enclosed within #ifdef 
>> INCLUDE_CDS_JAVA_HEAP and its usage in SymbolTable is enclosed within 
>> #ifdef INCLUDE_CDS.
>>
>> thanks,
>> Calvin
>>
>> On 9/18/18, 9:34 PM, Ioi Lam wrote:
>>> Hi Jiangli,
>>>
>>> Thanks for the review. Updated webrev:
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk12/8210875-refactor-compact-hash.v02/ 
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk12/8210875-refactor-compact-hash.v02.delta/ 
>>>
>>>
>>>
>>> On 9/18/18 6:47 PM, Jiangli Zhou wrote:
>>>> Hi Ioi,
>>>>
>>>> Looks good overall. Please see my comments below.
>>>>
>>>> - src/hotspot/share/classfile/compactHashtable.hpp
>>>>
>>>> 138 // CompactHashtable is used to stored the CDS archive's 
>>>> symbol/string tables.
>>>>
>>>> Typo: stored. It's a preexisting issue.
>>>>
>>>>
>>>
>>> Fixed
>>>
>>>> +  V if_equals(const K key, int len, u4 offset) const {
>>>> +    V value = DECODE(_base_address, offset);
>>>> +    if (EQUALS(value, key, len)) {
>>>> +      return value;
>>>> +    } else {
>>>> +      return NULL;
>>>> +    }
>>>> +  }
>>>>
>>>> +        V res = if_equals(key, len, entry[0]);
>>>> +        if (res != NULL) {
>>>> +          return res;
>>>> +        }
>>>>
>>>> The name of if_equals() function is a bit confusing. I was going to 
>>>> suggest a different name, but the function is only called in two 
>>>> places and both call sites could simply do the following without 
>>>> if_equals():
>>>>
>>>> V res = DECODE(_base_address, entry);
>>>> if (EQUALS(res, key, len)) {
>>>>   return res;
>>>> }
>>>>
>>>>
>>> That's a good suggestion. I further simplified it to:
>>>
>>>   V decode(u4 offset) const {
>>>     return DECODE(_base_address, offset);
>>>   }
>>>   ...
>>>
>>>
>>>         V value = decode(entry[0]);
>>>         if (EQUALS(value, key, len)) {
>>>           return value;
>>>         }
>>>
>>>
>>>> - src/hotspot/share/classfile/stringTable.cpp
>>>>
>>>> +inline bool string_equals_compact_hashtable_entry(oop value, const 
>>>> jchar* key, int len) {
>>>> +  return java_lang_String::equals(value, (jchar*)key, len);
>>>> +}
>>>>
>>>> Can java_lang_String::equals() be used for _shared_table for string 
>>>> directly? Or you simply want to mirror the 
>>>> symbol_equals_compact_hashtable_entry()?
>>>>
>>> I couldn't use java_lang_String::equals due to the mismatch of 
>>> "const jchar*" vs "jchar*" in the signature. You can see the awkward 
>>> (jchar*) type cast in the code above.
>>>
>>> But I think what you suggested is better, so I fixed all the missing 
>>> "const" in stringTable.hpp, javaClasses.hpp and utf8.hpp that are 
>>> related to Strings. Now I can use java_lang_String::equals directly 
>>> in the template instantiation.
>>>
>>>>
>>>> The _shared_table is only used for INCLUDE_CDS_JAVA_HEAP, can you 
>>>> please move it under INCLUDE_CDS_JAVA_HEAP?
>>>>
>>>>   77 static CompactHashtable<
>>>>   78   const jchar*, oop,
>>>>   79   read_string_from_compact_hashtable,
>>>>   80   string_equals_compact_hashtable_entry
>>>>   81 > _shared_table;
>>>>
>>>>
>>> Fixed.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>> On 9/18/18 10:35 AM, Ioi Lam wrote:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8210875
>>>>> http://cr.openjdk.java.net/~iklam/jdk12/refactor-compact-hash.v01/
>>>>>
>>>>> The template class CompactHashtable contains code for symbols
>>>>> and strings, which should really be provided by the instantiations
>>>>> of this template (i.e., {StringTable,SymbolTable}::_shared_table).
>>>>>
>>>>> I have refactored CompactHashtable to make it similar to
>>>>> ResourceHashtable.
>>>>>
>>>>> That way we can more easily create other instantiations, which 
>>>>> would be
>>>>> handy in JDK-8210388 (Use hash table to store archived subgraph_info
>>>>> records.)
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>
>>>
>