RFR(L): 8186842: Use Java class loaders for creating the CDS archive
Jiangli Zhou
jiangli.zhou at Oracle.COM
Wed Aug 30 23:57:26 UTC 2017
Hi Calvin,
Thank you for the additional changes and testing. Following are two minor issues from the latest webrev. No need for new webrev after you fix them.
Could you please change the following comment in klassFactory.cpp to be something more meaningful. How about “shared classes loaded by user defined class loader do not have shared_classpath_index”?
75 // AppCDSv2 class.
In the same file, please remove the ‘&& INCLUDE_JVMTI’ from line 230. The call to record_shared_class_loader_type() should always be done when CDS is enabled.
230 #if INCLUDE_CDS && INCLUDE_JVMTI
231 if (DumpSharedSpaces) {
<> 232 ClassLoader::record_shared_class_loader_type(result, stream);
233 #if INCLUDE_JVMTI
Thanks,
Jiangli
> On Aug 30, 2017, at 9:48 AM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
>
> Hi Jiangli,
>
> Thanks for your review.
>
> On 8/29/17, 12:55 PM, Jiangli Zhou wrote:
>>
>> 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?
> No. Because at the time of the initialization of _num_boot_entries, the _num_entries only has the number of entries in the boot class path including the runtime image. Later on, the _num_entries can change. Take a look at ClassLoader::add_to_list(ClassPathEntry *new_entry) and its callers.
>>
>> 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.
> That's a good idea.
>> 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 }
> It turns out it is no longer needed so I've removed it.
>> 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've made the change.
>> 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 })
>>
> Thanks Ioi for answering this one.
>> - 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();
> Yes, I'm using ik->module() in my new webrev.
>> 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
> Based on our off-list discussion, we've decided to keep the code but moving it further down in the same function together with another existing "#if INCLUDE_CDS" block.
>
> updated webrevs:
>
> incremental: http://cr.openjdk.java.net/~ccheung/8186842/webrev_00_01/ <http://cr.openjdk.java.net/~ccheung/8186842/webrev_00_01/>
> complete: http://cr.openjdk.java.net/~ccheung/8186842/webrev.01/ <http://cr.openjdk.java.net/~ccheung/8186842/webrev.01/>
>
> thanks,
> Calvin
>
>> Thanks,
>>
>> Jiangli
>>
>>> On Aug 28, 2017, at 10:34 AM, Calvin Cheung <calvin.cheung at oracle.com <mailto: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 <https://bugs.openjdk.java.net/browse/JDK-8186842>
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/ <http://cr.openjdk.java.net/%7Eccheung/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 <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
>>
More information about the hotspot-runtime-dev
mailing list