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

[coleen.phillimore at oracle.com]

Hi Calvin,
Your changes look good.  Thank you for answering my questions.  One 
minor change.

http://cr.openjdk.java.net/~ccheung/8186842/webrev_00_01/src/share/vm/classfile/classLoader.cpp.udiff.html

*!context.record_result(ik->name(), e, classpath_index, ik, _CATCH_);*

This should go back to THREAD because it's at the end of the function.  
Sorry for the confusion.

I don't need to see another webrev.  Thanks!

Coleen

On 8/30/17 1:04 PM, Calvin Cheung wrote:
> 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
>>