RFR(S) 8210875 Refactor CompactHashtable

[Ioi Lam]

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
>