RFR (M) 8213346 Re-implement shared dictionary

[Jiangli Zhou]

Hi Ioi,

Looks good. I have following minor comments. No new webrev is needed for 
me. The new shared system dictionary will help providing a cleaner 
approach for the dynamic archiving.

- Please wrap check_constraint_offset() with #ifdef ASSERT.

- Please fix the format for line 1113. Spaces are needed before & after 
= and <.

1113   for (int i=0; i<vc_array->length(); i++) {

Thanks,
Jiangli

On 11/13/18 12:48 PM, Ioi Lam wrote:
> 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
>>>
>>