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

[Jiangli Zhou]

Hi Ioi,

This looks good. Glad to see the change that utilizes the existing 
BasicHashtable. Thanks Coleen for the suggestion!

To avoid changing the existing assumptions about BasicHashtable and 
Hashtable, how about adding a destructor to the new KVHashtable instead 
of BasicHashtable? Just did a quick test, which seems to work properly.

Thanks,

Jiangli


On 11/16/18 7:55 PM, Ioi Lam wrote:
> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>