RFR(S) 8210388 Use hash table to store archived subgraph_info records

[Jiangli Zhou]

Hi Ioi,

The change looks clean. Here are my comments:

- src/hotspot/share/classfile/symbolTable.cpp

  654     if (fixed_hash == 0) {
  655       return true;
  656     }

It's been a long time since I touched this code. Can you please remind 
me when the result of hash_shared_symbol can be 0?

- src/hotspot/share/memory/heapShared.cpp

52 KlassSubGraphInfo* HeapShared::get_subgraph_info(Klass* k) {

Since you are here, can you please add an assert to make sure it's 
called at dump time only.

175     if (info.subgraph_object_klasses() != NULL || 
info.subgraph_entry_fields() != NULL) {

We should not have a subgraph_info with no object_klasses and no entry 
field. A subgraph_info can have no object_klasses, but should have at 
least one entry field. I think we should have an assert instead.

202   _run_time_subgraph_info_table.reset();

Why is the above needed in HeapShared::create_hashtables()?

HeapShared::create_hashtables() can use a better name. How about 
HeapShared::write_archived_subgraph_info_table()? 
HeapShared::serialize_hashtables() can also be renamed.

282         log_debug(cds, heap)("  %p init field @ %2d = %p", k, 
field_offset, (address)v);

It's better to use PTR_FORMAT.

- src/hotspot/share/memory/heapShared.hpp

120   class DumpTimeKlassSubGraphInfoTable
  121     : public ResourceHashtable<Klass*, KlassSubGraphInfo,
  122                                HeapShared::klass_hash,
  123                                HeapShared::klass_equals,
  124                                15889, // prime number
  125                                ResourceObj::C_HEAP> {
  126   public:
  127     int _count;
  128   };

A much smaller size can be used. Currently, we only have single digit 
subgraph infos. In the future, it might grow to hundreds infos.

Thanks,
Jiangli


On 9/20/18 4:46 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
> On 9/20/18 4:41 PM, Ioi Lam wrote:
>
>>
>>
>> On 09/20/2018 03:07 PM, Jiangli Zhou wrote:
>>> Hi Ioi,
>>>
>>>
>>> On 9/20/18 11:28 AM, Ioi Lam wrote:
>>>> http://cr.openjdk.java.net/~iklam/jdk12/8210388-hashtable-subgraph-info.v01/ 
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8210388
>>>>
>>>> Here's another small step towards CDS support for Lambda 
>>>> (JDK-8198698).
>>>>
>>>> Replace the linear search of ArchivedKlassSubGraphInfoRecord with
>>>> a hashtable. Currently we have just 6 such records, but with Lambda 
>>>> support
>>>> that number is going to increase substiantially.
>>>>
>>>>      Shared subgraphs table stats
>>>>      Number of entries       :         6
>>>>      Total bytes used        :        72
>>>>      Average bytes per entry :    12.000
>>>>      Average bucket size     :     6.000
>>>>      Variance of bucket size :     0.000
>>>>      Std. dev. of bucket size:     0.000
>>>>      Empty buckets           :         0
>>>>      Value_Only buckets      :         0
>>>>      Other buckets           :         1
>>>>
>>>> I also took the chance to clean up the use of 
>>>> CompactHashtableWriter in
>>>> the symbol/string tables. In doing so, I fixed a bug where
>>>> SymbolTable::copy_shared_symbol_table was skipping symbols with 
>>>> zero hashcodes.
>>>> This should be done for the string table only.
>>>>
>>>> Jiangli, could you help me with this comments?
>>>>
>>>>     unsigned int hash = java_lang_String::hash_code(s);
>>>>     if (hash == 0) {
>>>>       // We do not archive Strings with a 0 hashcode because ......
>>>>       return true;
>>>>     }
>>> The intent was to prevent possible write into the closed archive 
>>> region when identity_hash was called on an empty String. The empty 
>>> String is archived in the open archive heap region. This may not be 
>>> necessary as we always pre-compute identity hash for archived 
>>> object. If you want to remove this restriction, please be careful to 
>>> verify all possible cases.
>>>
>>
>> Hi Jiangli,
>>
>> Thanks for the explanation. I am not going to change the logic in the 
>> code quoted above. I just wanted to add a comment there to explain 
>> why we are doing that. Do we have an existing comment for this else 
>> where?
> Please do. Thanks. There is no existing comment, I think.
>>
>> I removed the (hash == 0) check in the symbol table dumping code, 
>> since that's a bug.
> I'll take a look of your change.
>
> Thanks,
> Jiangli
>>
>> Thanks
>> - Ioi
>>
>>
>>> Thanks,
>>> Jiangli
>>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>
>>
>