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

[Calvin Cheung]

Hi Ioi,

Thanks for the suggestion.

Based on our off-list discussion, I've made the change with a slight 
modification:
void ConstantPool::archive_resolved_references(Thread* THREAD) {
     ....
     for (int i = 0; i < rr_len; i++) {
       oop p = rr->obj_at(i);
+     rr->obj_at_put(i, NULL);
       if (p != NULL && i < ref_map_len) {

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

On 8/29/17, 10:00 PM, Ioi Lam wrote:
> Hi Calvin, there's one more place where we can get rid of 'continue'
>
> void ConstantPool::archive_resolved_references(Thread* THREAD) {
>     ....
>     for (int i = 0; i < rr_len; i++) {
>       oop p = rr->obj_at(i);
>       if (p != NULL && i < ref_map_len) {
> +       rr->obj_at_put(i, NULL);
>         int index = object_to_cp_index(i);
>         // Skip the entry if the string hash code is 0 since the string
>         // is not included in the shared string_table, see 
> StringTable::copy_shared_string.
>         if (tag_at(index).is_string() && 
> java_lang_String::hash_code(p) != 0) {
>           oop op = StringTable::create_archived_string(p, THREAD);
>           // If the String object is not archived (possibly too large),
>           // NULL is returned. Also set it in the array, so we won't
>           // have a 'bad' reference in the archived resolved_reference
>           // array.
>           rr->obj_at_put(i, op);
> -         continue;
>         }
>       }
> -     rr->obj_at_put(i, NULL);
>     }
>
> Thanks
> - Ioi
>
> On 8/29/17 5:18 PM, coleen.phillimore at oracle.com wrote:
>>
>> http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/src/share/vm/classfile/classLoader.cpp.udiff.html 
>>
>>
>> Can you put some comment like "Find if the class is from the runtime 
>> image" above this.  I couldn't guess reading this so used Jianli's 
>> review as a hint.
>>
>> *+ if ((strcmp(_jrt_entry->name(), src) == 0) ||*
>> *+ (module != NULL && (module->name() != NULL) &&*
>> *+ (strcmp(module->name()->as_C_string(), src) == 0))) {*
>> *+ e = _jrt_entry;*
>>
>>
>> Can you change this:
>>
>> *+ if (!get_canonical_path(e->name(), canonical_path, 
>> JVM_MAXPATHLEN)) {*
>> *+ continue;*
>> *+ }*
>> *+ if (strcmp(canonical_path, os::native_path((char*)src)) == 0) {*
>> *+ break;*
>> *+ }*
>> *+ classpath_index ++;*
>>
>> To:
>>
>> *+ if (get_canonical_path(e->name(), canonical_path, JVM_MAXPATHLEN)) {*
>> *+ if (strcmp(canonical_path, os::native_path((char*)src)) == 0) {*
>> *+ break;*
>> *+ }*
>> *+ classpath_index ++; + } *
>>
>>
>> So the confusing "continue" goes away?
>>
>> *+ const char* const class_name = ik->name()->as_C_string();*
>>
>>
>> I think you need another ResourceMark here.
>>
>> *+ ClassLoaderExt::Context context(class_name, file_name, THREAD);*
>> *+ context.record_result(ik->name(), e, classpath_index, ik, THREAD); 
>> // this is a tail call so doesn't need CATCH or CHECK *
>>
>> Could these throw exceptions and you don't expect them too?   Or do 
>> they just need a thread argument?  If the former, chagne THREAD to 
>> CATCH.
>>
>> *+ #endif*
>>
>>
>> Can you put what this is an #endif to as a comment since it's far 
>> away from the #if ?
>>
>> http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/src/share/vm/classfile/classLoaderExt.hpp.udiff.html 
>>
>>
>> *+oop h_loader = result->class_loader();*
>>
>>
>> Nit, can you remove h_ from the name since it's not a Handle.
>>
>> http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/src/share/vm/classfile/klassFactory.cpp.udiff.html 
>>
>>
>> *+ ClassLoaderData* loader_data = 
>> ClassLoaderData::class_loader_data(class_loader());*
>> *+ if (loader_data != NULL) {*
>> *+ pkg_entry = loader_data->packages()->lookup_only(pkg_name);*
>> *+ } *
>>
>> The ClassLoaderData should never be null at this point, and why would 
>> it be different than the one you fetched above.  I think this would 
>> be not legal to change the class loader with CFLH, and the original 
>> loader_data is used below, so this should be the same one.
>>
>> I think 12 or more inserted lines should be a new static function 
>> above, that's called here, like
>>    const char* pathname = get_package_name(loader_data, class_name, 
>> path_index, CHECK);
>>
>> http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/src/share/vm/classfile/systemDictionary.cpp.udiff.html 
>>
>>
>> The combining entries looks good.   I think it needs a comment that 
>> it's only done during dump time (or an assert).
>>
>> *+ Dictionary* master_dictionary = 
>> ClassLoaderData::the_null_class_loader_data()->dictionary();*
>>
>>
>> It's been bothering me that the shared dictionary at dump time is the 
>> NULL_CLD one.  With the combining, I think the dictionary at dump 
>> time should be shared_dictionary().   Can this be a follow on RFE to 
>> clean this up to use shared_dictionary()?   I think this change 
>> enables that.
>>
>> Do you have to free the initiating entries?   Can you leave them around?
>>
>> http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/src/share/vm/memory/metaspaceShared.cpp.udiff.html 
>>
>>
>> *+ NOT_PRODUCT(*
>> *+ static void assert_not_anonymous_class(InstanceKlass* k) {*
>> *+ if (k->is_instance_klass()) {*
>> *+ assert(!(k->is_anonymous()), "cannot archive anonymous classes");*
>> *+ }*
>> *+ }*
>>
>> You don't have to ask if k->is_instance_klass() since it passes in an 
>> InstanceKlass.
>>
>> It surprises me that there are no anonymous classes loaded (I'll read 
>> Ioi's reply later).  I don't know if that will remain the case though.
>>
>> *+ tty->print_cr("Preload Warning: Cannot find %s", 
>> parser.current_class_name());*
>>
>>
>> You should have an RFE to use log_warning() instead of tty->print_cr 
>> for all the CDS messages.
>>
>> http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/src/share/vm/oops/arrayKlass.cpp.udiff.html 
>>
>>
>> Because we use ClassLoaderDataGraph::classes_do() I thought all the 
>> array dimension Klasses are walked and this isn't needed.
>>
>> http://cr.openjdk.java.net/~ccheung/8186842/webrev.00/src/share/vm/oops/constantPool.cpp.udiff.html 
>>
>>
>> Why are you unresolveing Klasses?  I thought that was a good thing 
>> for performance.  Can you add a comment why?  There's some leftover 
>> #if0 code.
>>
>> I've completed my review and these are only minor comments and 
>> questions.  I might need to see an incremental or new review 
>> depending on how much you change.   It looks good.
>>
>> Thanks,
>> Coleen
>>
>>
>> On 8/28/17 1:34 PM, Calvin Cheung 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
>>
>