RFR: 8078644: CDS needs to support JVMTI CFLH

[Mikhailo Seledtsov]

I have sent out webrev for test changes for this work on a separate thread:
RFR(M): JDK-8165805 - [TESTBUG] CDS needs to support JVMTI CFLH - test 
development

Thank you,
Misha


On 9/9/16, 5:53 PM, Jiangli Zhou wrote:
> Hi Dan,
>
> Thanks for the review!
>
>> On Sep 9, 2016, at 3:40 PM, Daniel D. Daugherty<daniel.daugherty at oracle.com>  wrote:
>>
>> On 9/6/16 6: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/
>> General comment
>>
>> - please make sure all copyright years are updated.
>> - there are some jcheck space at end-of-line violations
>>   (There are more than what I flagged in the comments below).
> Will make sure to fix those before commit.
>
>> - I think there might be a data leak with the _cached_class_file
>>   field, but that pre-dates your changes and I have to think about it.
>>
>>
>> src/share/vm/oops/instanceKlass.hpp
>>     No comments.
>>
>> src/share/vm/oops/instanceKlass.cpp
>>     L2119:   // deallocate the cached class file
>>     L2120:   if (_cached_class_file != NULL) {
>>     L2121:     os::free(_cached_class_file);
>>     L2122:     _cached_class_file = NULL;
>>     L2123:   }
>>         Doesn't this use of _cached_class_file need to be checked
>>         against the shared spaces? I don't think os::free() will
>>         be happy if _cached_class_file points into shared space.
> That’s a good question. Only shared InstanceKlass might have _cached_class_file pointing to shared space. The deallocation call path never happen for shared classes at runtime. So if we get here, we must not be dealing with a shared class and _cached_class_file is not shared. Otherwise we would see issues for other data being freed in InstanceKlass::release_C_heap_structures().
>
>> src/share/vm/memory/metaspaceShared.hpp
>>     No comments.
>>
>> src/share/vm/memory/metaspaceShared.cpp
>>     No comments.
>>
>> src/share/vm/memory/metaspace.cpp
>>     No comments.
>>
>> src/share/vm/classfile/klassFactory.hpp
>>     No comments.
>>
>> src/share/vm/classfile/klassFactory.cpp
>>     L219:     if ((result->get_cached_class_file()) != NULL) {
>>     L220:       // JFR might set cached_class_file
>>     L221:       len = result->get_cached_class_file_len();
>>     L222:       bytes = result->get_cached_class_file_bytes();
>>     L223:     } else {
>>         Perhaps:
>>               if ((bytes = result->get_cached_class_file()) != NULL) {
>>                 // event based tracing might set cached_class_file
>>                 len = result->get_cached_class_file_len();
>>               } else {
>>
>>         so two changes:
>>           - mv init of bytes into the if-statement
>>           - change 'JFR' ->  'event based tracing’
> Updated.
>
>> src/share/vm/classfile/systemDictionary.cpp
>>     No comments.
>>
>> src/share/vm/memory/filemap.hpp
>>     L260:   }
>>     L261:   // The estimated optional space size. Only the portion containning data is
>>
>>         Please add a blank line between L260 and L261.
>>
>>         Typo: 'containning' ->  ‘containing'
> Fixed.
>
>>         There's a trailing space on L261; jcheck won't like it.
> Fixed.
>
>>     L266:   }
>>     L267:   // Total shared_spaces size includes the ro, rw, md, mc and od spaces
>>
>>         Please add a blank line between L266 and L267.
> Done.
>
>>     L264:   static size_t optional_space_size() {
>>     L265:     return core_spaces_size();
>>
>>         It is not clear why core_spaces_size() is being returned as
>>         an estimate of the optional space size.
> Coleen also pointed it out. I added more details.
>
>>         There's a trailing space on L265; jcheck won't like it.
> Fixed.
>
>> src/share/vm/memory/filemap.cpp
>>     No comments.
>>
>> src/share/vm/utilities/debug.hpp
>>     No comments.
>>
>> src/share/vm/utilities/debug.cpp
>>     L289:     "shared miscellaneous code space",
>>     L290:   };
>>         The name string for the new space is missing.
>>         Perhaps "shared optional space"…
> Also fixed when incorporating Coleen’s feedbacks. The estimated shared space size should be large enough and never run out space. Added a check.
>
> Thanks!
>
> Jiangli
>
>> Dan
>>
>>
>>>          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