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

[Ioi Lam]

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);
  + }


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

===============

For SystemDictionary::quick_resolve(), I think we should add an assert 
for k->is_shared().

===============

Is the check for MAX_QUICK_RESOLVE_ID necessary?

   if (UseSharedSpaces && !JvmtiExport::should_post_class_prepare()
                       && id > MAX_QUICK_RESOLVE_ID) {

===============

+  if ((*klassp) == NULL || (*klassp)->class_loader_data() == NULL) {

Maybe it's better to change to

    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(), ...);

===============

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.


Thanks!
- Ioi


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
>