RFR: 8078644: CDS needs to support JVMTI CFLH

Jiangli Zhou jiangli.zhou at oracle.com
Tue Sep 13 18:02:17 UTC 2016 

Hi Karen,

Thank you!

Just in case if you want to double check, here is the updated webrev with the add_package() fix for boot loader.

  http://cr.openjdk.java.net/~jiangli/8078644/webrev.02/

The KlassFactory::check_shared_class_file_load_hook() is updated to call add_package() for the boot loader. It is done right before running the ‘new_ik’ to make sure there is no other loading failure once the package is added. No other change was made to the webrev.

      if (class_loader.is_null()) {
        ResourceMark rm;
        ClassLoader::add_package(class_name->as_C_string(), path_index, THREAD);
      }

On the test side for this particular issue, I modified the new TransformSuperAndSubClasses test and verified the correct system packages were obtained by Package.getPackages(). I’ll work with Misha to incorporate the test update.

Thanks,
Jiangli

> On Sep 13, 2016, at 7:01 AM, Karen Kinnear <karen.kinnear at oracle.com> wrote:
> 
> Jiangli,
> 
> I finished my review and found the answers to my other questions - so all set!
> 
> thanks,
> Karen
> 
>> On Sep 12, 2016, at 7:26 PM, Jiangli Zhou <jiangli.zhou at oracle.com <mailto:jiangli.zhou at oracle.com>> wrote:
>> 
>> Hi Karen,
>> 
>> Thank you so much for the review!
>> 
>>> On Sep 12, 2016, at 1:58 PM, Karen Kinnear <karen.kinnear at oracle.com <mailto:karen.kinnear at oracle.com>> wrote:
>>> 
>>> Jiangli,
>>> 
>>> This looks great! Many thanks. Very creative solution!
>>> 
>>> Question:
>>> 
>>> If you load_shared_class for the boot loader and you use the CFLH such that you
>>> return the new_ik, where do you call ClassLoader::add_package?
>>> (I did find a call to notify_class_loaded from fill_instance_klass which is called by create_instance_klass)
>> 
>> Good catch!! In ClassLoader::load_class(), the add_packege() is done through ClassLoaderExt::Context::record_result(). For shared classes, SystemDictionary::load_shared_class() calls add_package() explicitly for boot loader:
>> 
>>     // For boot loader, ensure that GetSystemPackage knows that a class in this
>>     // package was loaded.
>>     if (class_loader.is_null()) {
>>       int path_index = ik->shared_classpath_index();
>>       ResourceMark rm;
>>       ClassLoader::add_package(ik->name()->as_C_string(), path_index, THREAD);
>>     }
>> 
>> We need to do that before returning the transformed class. Will fix.
>> 
>> Thanks!
>> 
>> Jiangli
>> 
>> 
>> 
>>> 
>>> thanks,
>>> Karen
>>> 
>>> p.s. I may have a couple more questions - I will try to get them to you tomorrow. Thanks.
>>> 
>>>> On Sep 9, 2016, at 11:42 PM, Jiangli Zhou <jiangli.zhou at oracle.com <mailto:jiangli.zhou at oracle.com>> wrote:
>>>> 
>>>> Here is the updated webrev.
>>>> 
>>>> 	http://cr.openjdk.java.net/~jiangli/8078644/webrev.01/ <http://cr.openjdk.java.net/~jiangli/8078644/webrev.01/>
>>>> 
>>>> Thanks everyone again for the timely feedbacks!
>>>> 
>>>> Jiangli
>>>> 
>>>>> On Sep 9, 2016, at 5:53 PM, Jiangli Zhou <jiangli.zhou at Oracle.COM <mailto:jiangli.zhou at oracle.com>> wrote:
>>>>> 
>>>>> Hi Dan,
>>>>> 
>>>>> Thanks for the review!
>>>>> 
>>>>>> On Sep 9, 2016, at 3:40 PM, Daniel D. Daugherty <daniel.daugherty at oracle.com <mailto: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 <https://bugs.openjdk.java.net/browse/JDK-8141341>>).
>>>>>>> 
>>>>>>> 	webrev:http://cr.openjdk.java.net/~jiangli/8078644/webrev.00/ <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 <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
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>

More information about the hotspot-runtime-dev mailing list