RFR(S) 8210875 Refactor CompactHashtable

[Ioi Lam]

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