RFR: JDK-8236604: Optimize SystemDictionary::resolve_well_known_classes for CDS

Yumin Qi yumin.qi at oracle.com
Fri Feb 28 20:08:55 UTC 2020 

Coleen,

On 2/28/20 12:05 PM, coleen.phillimore at oracle.com wrote:
>
> http://cr.openjdk.java.net/~minqi/8236604/webrev-01/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html 
>
> + oop class_loader = loader_data->class_loader();
> + if (class_loader == NULL) {
> This would be better as
>     if (loader_data->is_the_null_class_loader_data())
>
Thanks for catching this!

Yumin

> Then you don't have to touch the oop.
>
> Looks good apart from this one comment.  I don't need to see another 
> webrev if you fix this.
>
> Thanks,
> Coleen
>
> On 2/26/20 8:50 PM, Yumin Qi wrote:
>>
>> Hi, Ioi
>>
>>
>>   Thank for catching up those details.
>>
>> On 2/26/20 11:45 AM, Ioi Lam wrote:
>>> Hi Yumin,
>>>
>>> Looks good. Just small nits. No need for new webrev
>>>
>>> [1]
>>> Are you sure the check for "&& Object_klass_loaded()" is needed in 
>>> SystemDictionary::resolve_wk_klass()? I removed it and the VM seems 
>>> to work fine and passed all CDS tests.
>>
>> Yes, you are right.In java_lang_Class::restore_archived_mirror:
>>
>>
>> if (!SystemDictionary::Class_klass_loaded()) {
>> assert(fixup_mirror_list() != NULL, “fixup_mirror_list not initialized”);
>> fixup_mirror_list()->push(k);
>> return true;
>> }
>>
>> In old version, Class_klass_loaded(), we check if Class_klass is 
>> NULL, but with the _well_known_klasses serialization, it definitely 
>> is not NULL, so the new version check if not NULL then check if it is 
>> loaded too. The code is for guarding the old version.
>>
>>>
>>> [2]
>>> systemDictionaryShared.hpp - copyright needs to be updated to 2020
>>>
>> Done.
>>> [3]
>>> For SystemDictionary::quick_resolve(), I think we should add one 
>>> more assert so it won't be abused in the future
>>>   assert(!Universe::is_fully_initialized(), "we can make short cuts 
>>> only during VM initialization");
>>>
>> Added.
>>
>> I updated the webrev in
>>
>>
>> http://cr.openjdk.java.net/~minqi/8236604/webrev-01/
>>
>>
>> Thanks
>>
>> Yumin
>>
>>> Thanks
>>> - Ioi
>>>
>>> On 2/26/2020 8:55 AM, Yumin Qi wrote:
>>>>
>>>> Hi, Ioi and all
>>>>
>>>>    Updated webrev :http://cr.openjdk.java.net/~minqi/8236604/webrev-01/
>>>>
>>>>   Add back the assert for Compile_lock in add_to_hierarchy and its 
>>>> calling paths. Changed them to conditional guard with 
>>>> Universe::is_fully_initialized(), the testing result showed almost 
>>>> same as first version.
>>>>
>>>>   Test passed hs-tier1-4.
>>>>
>>>>   Thanks
>>>>
>>>>   Yumin
>>>>
>>>> On 2/25/20 9:05 AM, Yumin Qi wrote:
>>>>> Hi, Ioi
>>>>>
>>>>> On 2/24/20 11:04 PM, Ioi Lam wrote:
>>>>>> Hi Yumin,
>>>>>>
>>>>>> This looks good. A few small nits:
>>>>>>
>>>>>> For the assets related to Compile_lock, I think you can change 
>>>>>> them to:
>>>>>>
>>>>>>   + if (Universe::fully_initialized()) {
>>>>>>       assert_locked_or_safepoint(Compile_lock);
>>>>>>   + }
>>>>>>
>>>>>> That way you don't have to remove the asserts, and you can call
>>>>>> SystemDictionary::add_to_hierarchy without making a copy of the 
>>>>>> code.
>>>>>>
>>>>>> Also in add_to_hierarchy:
>>>>>>
>>>>>>  + if (Universe::fully_initialized()) {
>>>>>>      CodeCache::flush_dependents_on(k);
>>>>>>  + }
>>>>>>
>>>>> This can save changes to the asserts but will add instructions to 
>>>>> runtime though it is not much. Let me check the effect.
>>>>>>
>>>>>> systemnDictionary.hpp:
>>>>>>
>>>>>> It seems unnecessary to make add_to_hierarchy() a public method.
>>>>>>
>>>>>> systemnDictionary.cpp:
>>>>>>
>>>>>> add_package_service() -- this should be a member of 
>>>>>> SystemDictionary.
>>>>>> It's better to not move the code up, so we can minimize the
>>>>>> amount of code changes.
>>>>>>
>>>>>> Also, the name add_package_service() is kind of unclear. I can't 
>>>>>> think
>>>>>> of a better name. Since we are just splitting out the bottom half of
>>>>>> load_shared_class, how about
>>>>>>
>>>>>>      ik->restore_unshareable_info(loader_data, protection_domain, 
>>>>>> CHECK_NULL);
>>>>>>    }
>>>>>> +  load_shared_class0(ik, loader_data, CHECK_NULL);
>>>>>> +  return ik;
>>>>>> + }
>>>>>> +
>>>>>> +
>>>>>> + // This common code used by both load_shared_class() and 
>>>>>> quick_resolve().
>>>>>> + void SystemDictionary::load_shared_class0(InstanceKlass* ik,
>>>>>> +          ClassLoaderData* loader_data, TRAPS) {
>>>>>>   ik->print_class_load_logging(loader_data, NULL, NULL);
>>>>>>
>>>>> I don't know how to name it either, maybe load_shared_class_misc 
>>>>> for doing the rest of the logging, adding, printing work?
>>>>>> ===============
>>>>>>
>>>>>> For SystemDictionary::quick_resolve(), I think we should add an 
>>>>>> assert for k->is_shared().
>>>>> OK, will add.
>>>>>>
>>>>>> ===============
>>>>>>
>>>>>> Is the check for MAX_QUICK_RESOLVE_ID necessary?
>>>>>>
>>>>>>   if (UseSharedSpaces && !JvmtiExport::should_post_class_prepare()
>>>>>>                       && id > MAX_QUICK_RESOLVE_ID) {
>>>>>>
>>>>> This still in need. If the check removed, we will assert on shared 
>>>>> archive objects are not fixed. That is, the resolving of 
>>>>> Object_klass should not use quick_resolve. Fixing shared archive 
>>>>> objects needs Object fully loaded, but if we quick_resolve Object 
>>>>> class, the restoring un-shareable info will try to fix the mirror, 
>>>>> point to shared archive object heap. There, assert on shared 
>>>>> archive heap not fixed --- the fixation is after resolving 
>>>>> Object_klass. This part confused me so I use the macro here for a 
>>>>> guard, the Object_klass has to follow the old path for resolution. 
>>>>> Or it can be changed to !is_wk_klass_loaded(Object_klass)
>>>>>> ===============
>>>>>>
>>>>>> +  if ((*klassp) == NULL || (*klassp)->class_loader_data() == 
>>>>>> NULL) {
>>>>>>
>>>>>> Maybe it's better to change to
>>>>>>
>>>>>>    if (is_wk_klass_loaded(*klassp)) {
>>>>>>
>>>>> Do you mean
>>>>>
>>>>> if (!is_wk_klass_loaded(*klassp)) ?
>>>>>
>>>>>> ===============
>>>>>>  void SystemDictionary::resolve_well_known_classes(TRAPS) {
>>>>>> -  assert(WK_KLASS(Object_klass) == NULL, "well-known classes 
>>>>>> should only be initialized once");
>>>>>>
>>>>>> Maybe it's better to leave this assert here, but change to
>>>>>>
>>>>>>     assert(!Object_klass_loaded(), ...);
>>>>>>
>>>>> Yes, after the patch, the Object class is not NULL and not loaded 
>>>>> at this point, but for non shared vesion, have to check if 
>>>>> Object_klass_loaded() will encounter NULL pointer first.
>>>>>> ===============
>>>>>>
>>>>>> While you are changing 
>>>>>> SystemDictionary::resolve_well_known_classes(), these
>>>>>> comments are very old (we don't have perm gen anymore). Could you 
>>>>>> delete them?
>>>>>>
>>>>>> 2038   // These calls iterate over the objects currently in the 
>>>>>> perm gen
>>>>>> 2039   // so calling them at this point is matters (not before 
>>>>>> when there
>>>>>> 2040   // are fewer objects and not later after there are more 
>>>>>> objects
>>>>>> 2041   // in the perm gen.
>>>>>>
>>>>>>
>>>>> Sure
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>> Yumin
>>>>>
>>>>>
>>>>>> On 2/24/20 9:02 PM, Yumin Qi wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>    Please review fix:
>>>>>>>
>>>>>>>    Bug: https://bugs.openjdk.java.net/browse/JDK-8236604
>>>>>>>
>>>>>>>    Webrev:http://cr.openjdk.java.net/~minqi/8236604/webrev-00
>>>>>>>
>>>>>>>     Description: Optimize well known classes initialization for 
>>>>>>> CDS during jvm startup.
>>>>>>>
>>>>>>>     When run with CDS, initialize well known classes (95 
>>>>>>> classes) will call resolve_or_fail thus go though the resolve 
>>>>>>> functions, locks etc. The initialization of well-known classes 
>>>>>>> happens in fact in very early stage, those lock can be avoided. 
>>>>>>> The fix is serializing SystemDictionay::_well_known_klasses into 
>>>>>>> CDS  and in runtime restore them by avoiding call 
>>>>>>> resolve_or_fail. Since Compile_lock has to be held for 
>>>>>>> SystemDictionary::add_to_hierachy, the fix avoids calling it by 
>>>>>>> copying the code from it in SytemDictionary::quick_resolve, but 
>>>>>>> this way I have to remove two asserts which will assert on 
>>>>>>> Compile_lock. The reminding usage of the lock was added as 
>>>>>>> comments on the function declarations of 
>>>>>>> Klass::append_to_sibling_list and 
>>>>>>> InstanceKlass::add_implementor. The two functions calling paths 
>>>>>>> have been checked to make sure they are not used in other places.
>>>>>>>
>>>>>>>      Performance measured by take time before and after 
>>>>>>> SystemDictionary::resolve_well_known_classes(manually modified 
>>>>>>> orignal/new versions), since this is the most direct measurement 
>>>>>>> and accurate(excluded in review code):
>>>>>>>
>>>>>>>     + jlong s0 = os::javaTimeNanos();
>>>>>>>      resolve_well_known_classes(CHECK);
>>>>>>>
>>>>>>>      + jlong s1 = os::javaTimeNanos();
>>>>>>>
>>>>>>>      // print out s1 - s0
>>>>>>>
>>>>>>>      Run -version 2000 times for original/new versions 
>>>>>>> respectively and took the averages. The saving is about 18% 
>>>>>>> (2.9ms vs 2.4ms), it is not a big saving but still helps to 
>>>>>>> improve the startup time.
>>>>>>>
>>>>>>>     Testing: local jtreg test on linux-x86.
>>>>>>>
>>>>>>>     pending mach5 hs-tier1-4
>>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Yumin
>>>>>>>
>>>>>>
>>>
>

More information about the hotspot-runtime-dev mailing list