RFR (S) 8213587 - Speed up CDS dump time by using resizable hashtables
Jiangli Zhou
jiangli.zhou at oracle.com
Sun Nov 18 06:28:17 UTC 2018
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.
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