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

[coleen.phillimore at oracle.com]

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