RFR (S) 8213587 - Speed up CDS dump time by using resizable hashtables

[Ioi Lam]

Hi Coleen,

I deleted the default value for MEMFLAGS as you suggested. For my 
instantiated templates, I still use mtInternal, though, since I can't 
find anything better for using at CDS dump time.

Also, Jiangli noted that there's a memory leak, because I allocate and 
free the KVHashtable dynamically. So I added a destructor to 
BasicHashtable to free the buckets and the block-allocated entries.

http://cr.openjdk.java.net/~iklam/jdk12/8213587-resize-cds-hashtables.v03/

http://cr.openjdk.java.net/~iklam/jdk12/8213587-resize-cds-hashtables.v03-delta/

This comment around Hashtable::allocate_new_entry() is wrong now -- "The 
allocator in blocks is preferable but doesn't have free semantics". 
Maybe I should change it to this?

"The block allocator in BasicHashtable has less fragmentation, but the 
memory is not freed until the whole table is freed. Use 
allocate_new_entry() if you want to immediately free the memory used by 
each entry".

I am rerunning hs-tiers{1,2,3,4} to catch any issues. I also tested the 
solaris/x64 build since it seems to break every time you do something 
with templates :-(

Thanks!
- Ioi

On 11/16/18 1:36 PM, coleen.phillimore at oracle.com wrote:
>
> Hi Ioi,  I really like this new wrapper on the old hashtable to not 
> have to write the boilerplate code!
>
> http://cr.openjdk.java.net/~iklam/jdk12/8213587-resize-cds-hashtables.v02/src/hotspot/share/utilities/hashtable.hpp.udiff.html 
>
>
> + MEMFLAGS F = mtInternal,
>
>
> I think you should require the mt type though and not make it a 
> default parameter. mtInternal is not very useful to finding memory leaks.
>
> Apart from this (which I don't need to see another version), your 
> change looks good and nice to get good performance benefits from this.
>
> thanks,
> Coleen
>
> On 11/15/18 12:31 AM, Ioi Lam wrote:
>> Coleen pointed out to me off-line that the good old (and ugly) 
>> BasicHashtable already supports resizing. I think that might be a 
>> better starting point for this RFE:
>>
>> http://cr.openjdk.java.net/~iklam/jdk12/8213587-resize-cds-hashtables.v02/ 
>>
>>
>> I wrote a new template class called "KVHashtable" (copying the style 
>> from ResourceHashtable). That way, you can instantiate different (Key 
>> -> Value) mappings without writing tons of boilerplate code. The 
>> performance is similar to my previous version, and the code is much 
>> cleaner.
>>
>> I also renamed the RFE title, as well as the subject line of this RFR 
>> e-mail.
>>
>> Thanks
>>
>> - Ioi
>>
>>
>> On 11/14/18 5:14 PM, Jiangli Zhou wrote:
>>> Hi Ioi,
>>>
>>> On 11/14/18 9:09 AM, Ioi Lam wrote:
>>>>
>>>>
>>>> On 11/13/18 4:05 PM, Jiangli Zhou wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> The change looks reasonable to me in general. It would be helpful 
>>>>> to see the performance difference with the expendable table. Do 
>>>>> you have any data when large number of classes are loaded 
>>>>> (>20000)? How much saving does it provide?
>>>>>
>>>>
>>>> Hi Jiangli, thanks for the review. For dumping 30292 classes:
>>>>
>>>> BEFORE: 93.971 sec
>>>> AFTER:  34.761 sec
>>>
>>> Thanks for the data! That's about 2.6x improvement with large set of 
>>> classes.
>>>
>>> Thanks,
>>> Jiangli
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>> Thanks,
>>>>>
>>>>> Jiangli
>>>>>
>>>>>
>>>>> On 11/8/18 10:35 PM, Ioi Lam wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213587
>>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8213587-configurable-resource-hash.v01/ 
>>>>>>
>>>>>>
>>>>>> TL;DR: -- add a subclass to ResourceHashtable to allow the table 
>>>>>> size to be
>>>>>>           dynamically specified when the table is constructed.
>>>>>>
>>>>>>
>>>>>>         *** C++ template guru alert ***
>>>>>>
>>>>>>
>>>>>> I don't know much about C++ templates, so my attempt on doing 
>>>>>> this may be
>>>>>> ill-advised.
>>>>>>
>>>>>> I *think* that with my patch, the performance of existing code, 
>>>>>> which uses
>>>>>> a statically-defined SIZE,  should not be affected, as the C++ 
>>>>>> compiler
>>>>>> should be able to constant-propagate and reduce the new code:
>>>>>>
>>>>>>   ALWAYSINLINE unsigned size() const {
>>>>>>     if (SIZE != CONFIGURABLE_SIZE) {
>>>>>>       return SIZE;
>>>>>>     } else {
>>>>>>       return _configured_table_size;
>>>>>>     }
>>>>>>   }
>>>>>>
>>>>>>   ALWAYSINLINE Node** get_table() const {
>>>>>>     if (SIZE != CONFIGURABLE_SIZE) {
>>>>>>       return (Node**)(&_static_table[0]);
>>>>>>     } else {
>>>>>>       return _configured_table;
>>>>>>     }
>>>>>>   }
>>>>>>
>>>>>>   Node** lookup_node(unsigned hash, K const& key) {
>>>>>>     unsigned index = hash % size();    <-----
>>>>>>     Node** table = get_table();
>>>>>>     Node** ptr = &table[index];   <-----
>>>>>>
>>>>>> back to the old code:
>>>>>>
>>>>>>   Node** lookup_node(unsigned hash, K const& key) {
>>>>>>     unsigned index = hash % SIZE;      <-----
>>>>>>     Node** ptr = &_table[index];       <-----
>>>>>>
>>>>>>
>>>>>> If anyone has a better way of doing this, I'd love to hear it!
>>>>>>
>>>>>> Thanks!
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>