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

[Calvin Cheung]

Hi Coleen,

Thanks for taking a look again.
I'll make the one-line change.

Calvin

On 8/30/17, 1:02 PM, coleen.phillimore at oracle.com wrote:
>
> 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
>>>
>