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

[Calvin Cheung]

Hi Coleen,

Thanks for your review.

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;*
>
I've added a comment. The conditions have been simplified per Jiangli's 
suggestion.
>
> 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?
I've fixed it per your suggestion.
>
> *+ const char* const class_name = ik->name()->as_C_string();*
>
>
> I think you need another ResourceMark here.
Added.
>
> *+ 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.
I've changed the TREAD to CATCH in both lines.
>
> *+ #endif*
>
>
> Can you put what this is an #endif to as a comment since it's far away 
> from the #if ?
Done.
>
> 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.
Done.
>
> 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);
The above have been changed to using ik->module() per Jiangli's suggestion.
>
> 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).
There's already an assert in the caller:
void SystemDictionary::combine_shared_dictionaries() {
   assert(DumpSharedSpaces, "dump time only");
>
> *+ 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.
Yes, we can file an RFE to investigate this.
>
> Do you have to free the initiating entries?   Can you leave them around?
If I don't free the entries, it will hit the following assert in 
BasicHashtable<F>::verify_table

   guarantee(number_of_entries() == element_count,
             "Verify of %s failed", table_name);
>
> 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.
Fixed.
>
> 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.
I've filed https://bugs.openjdk.java.net/browse/JDK-8186988
>
> 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.
This would involve mostly removing code in a few files but requires 
running a lot of tests to make sure the change is good.
I'll file a follow-up RFE to clean up the code.
>
> 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.
Added comment and removed the #if0 block in the new webrev.
>
> 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.
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,
> 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
>