RFR (S) 8213587 - Speed up CDS dump time by using resizable hashtables
Ioi Lam
ioi.lam at oracle.com
Tue Nov 20 05:25:24 UTC 2018
On 11/19/18 7:31 AM, coleen.phillimore at oracle.com wrote:
>
> Hi, Jiangli - Good find that the blocks leak! None of the tables that
> used blocks were ever deallocated, before this one. It might make
> sense for all the tables to use blocks with this change, although the
> blocks might be too big for some of the smaller hashtables.
>
The blocks are deallocated only when the table is free, but some tables
may want to free their entries immediately, or at least more eagerly.
I've filed https://bugs.openjdk.java.net/browse/JDK-8214030. I think we
should consolidate the code for allocating/freeing the entries.
Currently every table does its own way and that's pretty fragile.
Thanks
- Ioi
> Ioi, his change and the deltas look really good.
>
> Thanks,
> Coleen
>
> On 11/18/18 10: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