RFR (M) 8213346 Re-implement shared dictionary

[Ioi Lam]

Hi Jiangli,

Thanks for the reviews. Here's the updated patch:

http://cr.openjdk.java.net/~iklam/jdk12/8213346-shared-dict-using-compact-hashtable.v02/

http://cr.openjdk.java.net/~iklam/jdk12/8213346-shared-dict-using-compact-hashtable.v02.delta/


Please see my comments inline.


On 11/12/18 9:52 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
> I came across the SystemDictionary::clear_invoke_method_table() call 
> in MetaspaceShared::preload_and_dump() while looking into dynamic 
> dumping. With your shared dictionary change, I think 
> clear_invoke_method_table() is no longer needed anymore. Could you 
> please check that?
>
Yes, this function is not needed anymore. I've removed it.


>
> On 11/9/18 12:22 PM, Jiangli Zhou wrote:
>> Hi Ioi,
>>
>> This looks clean overall! I only have the following comments for 
>> systemDictionaryShared.cpp.
>>
>> - share/classfile/systemDictionaryShared.cpp
>>
>> _num_constraints * 2
>>
>> Please use a macro or a static function for the various 
>> '_num_constraints * 2', which is less error-prone and easier to 
>> maintain.
>>

I've added new structures DTConstraint and RTConstraint so "* 2" is not 
needed anymore.


>>  287 inline bool my_record_equals_compact_hashtable_entry
>>
>> How about renaming the above to klass_equals?

I moved this function into RunTimeSharedClassInfo::EQUALS and added 
comments.


>>
>>
>>  878 ResourceHashtable<
>>  879   Symbol*, bool,
>>  880   primitive_hash<Symbol*>,
>>  881   primitive_equals<Symbol*>,
>>  882   6661,                             // prime number
>>  883   ResourceObj::C_HEAP> _unregister_class_exists;
>>
>> Maybe rename _unregister_class_exists to _existing_unregister_classes 
>> or _loaded_unregister_class?
>>

I renamed to _loaded_unregistered_classes


>>  942 void 
>> SystemDictionaryShared::set_shared_class_misc_info(InstanceKlass* k, 
>> ClassFileStream* cfs) {
>>
>> Please add an assert to make sure it's only called for classes loaded 
>> by the non-builtin loaders.
>>

Added


>>  978     tty->print_cr("Preload Warning: Skipping %s from unsupported 
>> location",
>>  979                   k->name()->as_C_string());
>>  987     tty->print_cr("Preload Warning: Skipping %s from signed JAR",
>>  988                   k->name()->as_C_string());
>>  997     tty->print_cr("Skipping JFR event class %s", 
>> k->name()->as_C_string());
>>
>> We should consider using log_warning for above tty->print.
>>

Done. I added SystemDictionaryShared::warn_excluded which calls 
log_warning(cds).


Thanks

- Ioi


>> Thanks,
>>
>> Jiangli
>>
>>
>> On 11/8/18 10:04 PM, Ioi Lam wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8213346
>>> http://cr.openjdk.java.net/~iklam/jdk12/8213346-shared-dict-using-compact-hashtable.v01/ 
>>>
>>>
>>> Please review a long overdue cleanup of how the classes are indexed 
>>> in the CDS
>>> archive.
>>>
>>> [1] Use CompactHashtable which is more space efficient than Dictionary.
>>>
>>> [2] Space saving:
>>>
>>>     + For each shared class, we used to store a fixed-size record in
>>>       SharedDictionaryEntry. That information is now stored in a
>>>       variable-length structure (see RunTimeSharedClassInfo).
>>>
>>>     + Verifier constraints are now stored using u4 instead of Symbol*.
>>>
>>> Total space saving is about 45KB for the default CDS archive.
>>>
>>> [4] We had scattered code that checked whether a class should be 
>>> archived
>>>     or not. E.g., checking for code signers, JFR, etc. Now this is 
>>> checked inside
>>>     a consolidated function 
>>> SystemDictionaryShared::should_be_excluded()
>>>
>>> [5] Now we have 2 separate hashtable to look up shared classes: one 
>>> for the
>>>     built-in classes and the other for unregistered classes. This 
>>> simplifies the
>>>     lookup logic and is also faster.
>>>
>>> [6] A lot of old code was deleted. There's a net change of about 
>>> -250 lines of C
>>>     code.
>>>
>>> TESTING:
>>>
>>> I am running hs-tiers 1/2/3/4
>>>
>>> Thanks
>>> - Ioi
>>
>