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

[Ioi Lam]

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
>