RFR: 8078644: CDS needs to support JVMTI CFLH

[Coleen Phillimore]

http://cr.openjdk.java.net/~jiangli/8078644/webrev.00/src/share/vm/classfile/klassFactory.cpp.udiff.html

+ instanceKlassHandle nh = instanceKlassHandle();


You can just use NULL now.  The code that creates nullHandles will be 
cleaned up.
Thank you for putting this function in this place.

http://cr.openjdk.java.net/~jiangli/8078644/webrev.00/src/share/vm/classfile/systemDictionary.cpp.udiff.html

+ instanceKlassHandle new_ik = 
KlassFactory::check_shared_class_file_load_hook(
+ ik, class_name, class_loader, protection_domain, CHECK_(nh));

Here the InstanceKlass ik is read out of the archive, so you don't want 
to deallocate it.  Is this right?

http://cr.openjdk.java.net/~jiangli/8078644/webrev.00/src/share/vm/memory/filemap.hpp.udiff.html

+ // The estimated optional space size. Only the portion containning 
data is
+ // written out to the archive and mapped at runtime. There is no 
memory waste
+ // due to unused portion in optional space.
+ static size_t optional_space_size() {
+ return core_spaces_size();
+ }


Why is optional_spaces_size the same as core_spaces_size() ?  Is that 
just what it turned out to be?  I guess it would be annoying to have 
another OptionalSharedSpacesSize parameter?  Can the comment include why 
this size was picked?  There's an extra 'n' in containing in the comment.

http://cr.openjdk.java.net/~jiangli/8078644/webrev.00/src/share/vm/utilities/debug.cpp.udiff.html

This seems to be missing the string for optional_data.

http://cr.openjdk.java.net/~jiangli/8078644/webrev.00/src/share/vm/memory/metaspaceShared.cpp.html

Can you remove the lines, which are no longer true (I was looking for 
how MetaspaceShared::is_in_shared_space()) is defined with the od space.

1075 // Need to keep the bounds of the ro and rw space for the Metaspace::contains
1076 // call, or is_in_shared_space.

So it looks like here you always map in the od section?   The space 
isn't getting paged in unless it's used, but it is mapped in, true? Are 
there plans in 10 to not map it in?

I think you need
    char* od_base; in order to unmap the od section if the checksum fails.

+ mapinfo->map_region(od) != NULL && mapinfo->verify_region_checksum(od)) {


Everything else looks good except these minor comments and questions.

Thanks,
Coleen

On 9/6/16 8:14 PM, Jiangli Zhou wrote:
> Hi,
>
> Please review the following change thats support JVMTI class_file_load_hook(CFLH) during initial loading of shared classes. The webrev also removes the temporary workaround that disables CDS when JVMTI CFLH is required (JDK-8141341 <https://bugs.openjdk.java.net/browse/JDK-8141341>).
>
> 	webrev:http://cr.openjdk.java.net/~jiangli/8078644/webrev.00/
>          bug: JDK-8078644 <https://bugs.openjdk.java.net/browse/JDK-8078644>
>
> Class file data is needed when posting CFLH. For shared classes, the class file data are now archived at dump time. That avoids re-reading the class file data from the JAR file or reconstituting the data at runtime, which would add class loading overhead for shared classes.
>
> To minimize the runtime memory overhead, the archived class file data are stored in a separate shared space, called ‘optional data space’ (od).  The ‘od’ space a new region in the archive. It does not increase runtime memory usage when JvmtiExport::should_post_class_file_load_hook() is false. The memory contains the archived class file data is only paged in when the VM needs to post CFLH.  The ‘od’ space can be shared by different processes.
>
> When loading shared class at runtime, we now call JvmtiExport::post_class_file_load_hook() with the archive class file data if needed. If JVMTI agent modifies the class, new InstanceKlass is created using the modified class data and the archived class is ignored.
>
> Tested with JPRT, CDS tests and QA tests.
>
> Thanks,
> Jiangli