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

Ioi Lam ioi.lam at oracle.com
Sun Nov 18 02:48:59 UTC 2018



On 11/17/18 1:54 PM, Jiangli Zhou wrote:
> 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.
>
Hi Jiangli,

Thanks for the review.

Not freeing the entries when a BasicHashtable is destructed is a bug. 
None of the existing BasicHashtables (and subclasses thereof) are ever 
destructed, so I don't think my code would impact them. Also, if any 
code depends on the entries not freed even if the table is destructed, 
that's clearly a bug in that code, and it should be fixed.

If I don't add the destructor to BasicHashtable, the next person who 
wants to allocate/free BasicHashtables will run in to the same issue.

Thanks
- Ioi


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



More information about the hotspot-dev mailing list