RFR (M) 8213346 Re-implement shared dictionary

[Ioi Lam]

Hi Jiangli, thanks for the review.


On 11/13/18 1:10 PM, Jiangli Zhou wrote:
> 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.
>
The C++ compiler will eliminate the call to check_constraint_offset() in 
product build. Adding #ifdef here will just add unnecessary bloat.

> - Please fix the format for line 1113. Spaces are needed before & 
> after = and <.
>
> 1113   for (int i=0; i<vc_array->length(); i++) {
>
Fixed.

Thanks
- Ioi

> 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
>>>>
>>>
>