RFR (S) 8213587 - Speed up CDS dump time by using resizable hashtables
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Nov 19 15:31:27 UTC 2018
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.
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