RFR: 8078644: CDS needs to support JVMTI CFLH

[Jiangli Zhou]

Hi Coleen,

Thanks for the review!

> On Sep 9, 2016, at 9:06 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> 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.

Remove the use of nh.

> 
> 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?

Yes. The ‘ik' is from mapped archive.

> 
> 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?

The core_spaces_size is the size of all class metadata (ro+rw+mc+md, not including the shared String objects). So that's a good estimate for the total class bytes size since the optional space only has the archived class bytes currently.

>  I guess it would be annoying to have another OptionalSharedSpacesSize parameter?

Yes, it does not worth to have another command-line option when we have a good estimated size that’s large enough for all archived class bytes.

>  Can the comment include why this size was picked?  

Added.

> There's an extra 'n' in containing in the comment.

Fixed. Thanks for catching that.

> 
> 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.

Thanks for catching that. The 'od' should never be out of space. I added a check in report_out_of_shared_space().

> 
> 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.

Removed.

> 
> 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?

Yes, that’s the case.

> Are there plans in 10 to not map it in?

Yes, as an optimization we want to not map the ‘od’ space by default. It should only be mapped when VM needs to post CFLH. When the ‘od’ space is not mapped at VM initialization but mapped at later time, the memory address that the ‘od’ space was allocated originally might be used before the mapping takes place. The ‘od’ space needs to be mapped at arbitrary location, so we need to make sure all data within ‘od’ is relocatable. Currently, that is the case.

> 
> 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)) {
> 
Fixed.

Thanks!
Jiangli

> 
> 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
>