RFR(L): 8186842: Use Java class loaders for creating the CDS archive

[Jiangli Zhou]

Hi Calvin,

These changes look good. I have a few remaining comments below.

- src/share/vm/classfile/classLoader.cpp
The ‘_num_boot_entries' variable probably is unnecessary. It’s initialized to _num_entries and never modified. In places where _num_boot_entries is used, can you use _num_entries directly?

I think we should remove the special boot classpath handling code for dump time. With the use of java class loaders at CDS/AppCDS dump time, we no longer need to append the -cp path to the boot classpath. With that, we can remove the CDS special cases from ClassLoader::load_class, ClassPathImageEntry::open_stream, etc. That would make the generic class loading code much cleaner. Since you are planning to integrate this change soon, making the suggested change can be risky. I’ll file a new RFE, we can do the clean up separately.
150 int             ClassLoader::_num_boot_entries   = -1;
1519       if (DumpSharedSpaces && classpath_index >= _num_boot_entries) {
1520         // Do not load any class from the app classpath using the boot loader. Let
1521         // the built-in app class laoder load them.
1522         break;
1523       }    
1635   if (classpath_index < _num_boot_entries) {
1636     // ik is either:
1637     // 1) a boot class loaded from the runtime image during vm initialization (classpath_index = 0); or
1638     // 2) a user's class from -Xbootclasspath/a (classpath_index > 0)
1639     // In the second case, the classpath_index, classloader_type will be recorded via
1640     // context.record_result() in ClassLoader::load_class(Symbol* name, bool search_append_only, TRAPS).
1641     if (classpath_index > 0) {
1642       return;
1643     }
1644   }

I’m wondering why the following is never needed before in get_package_entry() for non-CDS case. Do you have additional details?
 253   // PackageEntryTable could be NULL for classes like java/lang/invoke/LambdaForm$MH
 254   if (pkgEntryTable == NULL) {
 255     return NULL;
 256   }
If I understand it correctly, the following is to find if the class is from the runtime image. Can you please change the following to check with module->location()->starts_with(“jrt:”)?
1610   if ((strcmp(_jrt_entry->name(), src) == 0)  ||
1611       (module != NULL && (module->name() != NULL) &&
1612         (strcmp(module->name()->as_C_string(), src) == 0))) {
1613     e = _jrt_entry;
1614     classpath_index = 0;
I was looking for code that handles anonymous classes. There are following code in classLoader.cpp and metaspaceShared.cpp. However, I can’t find any code that specifically removes anonymous classes from the system dictionary at CDS dump time. I’m probably missing something, how do we guarantee (besides the assert) anonymous classes are not being archived?
1582 void ClassLoader::record_shared_class_loader_type(InstanceKlass* ik, const ClassFileStream* stream) {
1583   assert(DumpSharedSpaces, "sanity");
1584   assert(stream != NULL, "sanity");
1585 
1586   if (ik->is_anonymous()) {
1587     // We do not archive anonymous classes.
1588     return;
1589   }
487 NOT_PRODUCT(
 488 static void assert_not_anonymous_class(InstanceKlass* k) {
 489   if (k->is_instance_klass()) {
 490     assert(!(k->is_anonymous()), "cannot archive anonymous classes");
 491   }
 492 }
 494 static void assert_no_anonymoys_classes_in_dictionaries() {
 495   ClassLoaderDataGraph::dictionary_classes_do(assert_not_anonymous_class);
 496 })

- src/share/vm/classfile/klassFactory.cpp
 The following code can be simplified to get the module from ‘ik’.
  74       if (path_index < 0) {
  75         // AppCDSv2 class.
  76         // Get the pkg_entry from the classloader
  77         PackageEntry* pkg_entry = NULL;
  78         TempNewSymbol pkg_name = InstanceKlass::package_from_name(class_name, CHECK_NULL);
  79         if (pkg_name != NULL) {
  80           const char* pkg_string = pkg_name->as_C_string();
  81           ClassLoaderData* loader_data = ClassLoaderData::class_loader_data(class_loader());
  82           if (loader_data != NULL) {
  83             pkg_entry = loader_data->packages()->lookup_only(pkg_name);
  84           }
  85         }
  86         if (pkg_entry != NULL) {
  87           ModuleEntry* mod_entry = pkg_entry->module();
Could you please remove the following from KlassFactory::create_from_stream() and call it from ClassLoaderExt::record_result()?
 229 #if INCLUDE_CDS
 230   if (DumpSharedSpaces) {
 231     ClassLoader::record_shared_class_loader_type(result, stream);
 232   }
 233 #endif
Thanks,

Jiangli

> On Aug 28, 2017, at 10:34 AM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
> 
> Hi,
> 
> This is a re-post of a previous RFR for 8172218 using the correct bug id.
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8186842
> 
> webrev: http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/
> 
> Please refer to the comment <https://bugs.openjdk.java.net/browse/JDK-8186842?focusedCommentId=14113187&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14113187> section of the bug for description of the change.
> 
> Tests executed so far:
>    JPRT
>    hs-tier2 though hs-tier4
>    hs-tier5 (linux-x64)
> 
> thanks,
> Calvin