RFR(L): 8186842: Use Java class loaders for creating the CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Thu Aug 31 15:22:20 UTC 2017
Hi Jiangli,
Thanks for another round of review.
Nice catch on the #if statement.
I've made those changes and added a missing ResourceMark in the same file.
I've an incremental webrev if you want to take a look.
http://cr.openjdk.java.net/~ccheung/8186842/webrev.01_plus/
(ignore the cpCache.cpp change, I dont' know why the closing brace shows
up this time)
thanks,
Calvin
On 8/30/17, 4:57 PM, Jiangli Zhou wrote:
> 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
>> <mailto: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/
>> 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