RFR(L): 8186842: Use Java class loaders for creating the CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Wed Aug 30 16:48:37 UTC 2017
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/
complete: 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
>>
>> 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