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

Jiangli Zhou jiangli.zhou at oracle.com
Mon Nov 19 18:12:39 UTC 2018


Hi Ioi,

The update looks good.

Thanks,

Jiangli


On 11/18/18 7:50 PM, Ioi Lam wrote:
>
>
> On 11/17/18 10:28 PM, Jiangli Zhou wrote:
>> Hi Ioi,
>>
>> On 11/17/18 6:48 PM, Ioi Lam wrote:
>>
>>>
>>>
>>> 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.
>>
>> Dictionary, PackageEntryTable, ModuleEntryTable and 
>> G1CodeRootSetTable are derived from Hashtable and they call 
>> free_buckets() in their destructors. So free_buckets() will be called 
>> twice when those tables are freed although it's harmless. If you want 
>> to avoid the duplicated call, you could remove the free_buckets() 
>> calls from those classes' destructors and keep the current 
>> BasicHashtables changes. I don't have a strong opinion on this, I'll 
>> leave it to you to decide.
>>
>>
>
> Hi Jiangli,
>
> Good catch. I've remove the call to free_buckets() in the destructor 
> of those 4 classes. Here's the delta from the last webrev:
>
> http://cr.openjdk.java.net/~iklam/jdk12/8213587-resize-cds-hashtables.v04-delta/ 
>
>
> Also, I found 2 problems while doing this. Hashtables in HotSpot are 
> really a mess.
>
> https://bugs.openjdk.java.net/browse/JDK-8214029
> https://bugs.openjdk.java.net/browse/JDK-8214030
>
> Thanks
> - Ioi
>
>> Thanks,
>>
>> Jiangli
>>>
>>> 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