RFR: 8249262: Initialize InstanceKlass::_package_entry during CDS dump time [v2]

[Calvin Cheung]

On Mon, 25 Jan 2021 17:55:02 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> src/hotspot/share/oops/instanceKlass.cpp line 2558:
>> 
>>> 2556:     _package_entry = NULL;
>>> 2557:   }
>>> 2558: #endif
>> 
>> I think the `#if` part is not needed. `MetaspaceShared::use_full_module_graph()` returns false if `INCLUDE_CDS_JAVA_HEAP` is false.
>> 
>> Instead of `ArchiveBuilder::update_package_entry()`, maybe we can set `_package_entry` here?
>> 
>> if (!MetaspaceShared::use_full_module_graph()) {
>>   _package_entry = NULL;
>> } else if (DynamicDumpSharedSpaces) {
>>   if (!Metaspace::is_in_shared_metaspace(_package_entry)) {
>>     _package_entry = NULL;  
>>   }
>> } else {
>>   _package_entry =  PackageEntry::get_archived_entry(entry);
>> }
>> ArchiveUtils::mark_pointer((address**)&_package_entry);
>> 
>> The `mark_pointer` call is necessary if relocation happens during dumping.
>> 
>> For classes loaded by custom loaders, we don't archive their modules so get_archived_entry() should return a NULL package. I think for sanity, we should add an assert like:
>> 
>> if (ik->is_shared_unregistered_class()) {
>>   assert(_package_entry == NULL, "package entry for unregistered classes are not archived");
>> }
>
> Also need to assert `_package_entry == NULL` if the class is in an unnamed module.

Hi Ioi,

Thanks for your review.
As discussed off-line, instead of adding assert, the `else` block of the code will be as follows:
    if (is_shared_unregistered_class()) {
      _package_entry = NULL;
    } else {
      _package_entry = PackageEntry::get_archived_entry(_package_entry);
    }
I've made other changes based on your comments. Please see updated webrev 01.

thanks,
Calvin

-------------

PR: https://git.openjdk.java.net/jdk/pull/2206