RFR(S) 8210875 Refactor CompactHashtable

[Calvin Cheung]

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