RFR: 8078644: CDS needs to support JVMTI CFLH
Jiangli Zhou
jiangli.zhou at oracle.com
Wed Sep 14 02:11:54 UTC 2016
Hi Dan,
Thank you for double checking this!
> On Sep 13, 2016, at 7:00 PM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
>
> On 9/13/16 12:02 PM, Jiangli Zhou wrote:
>> 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/
>
> src/share/vm/classfile/klassFactory.cpp
> No comments.
>
> src/share/vm/classfile/klassFactory.hpp
> No comments.
>
> src/share/vm/classfile/systemDictionary.cpp
> No comments.
>
> src/share/vm/memory/filemap.cpp
> No comments.
>
> src/share/vm/memory/filemap.hpp
> No comments.
>
> src/share/vm/memory/metaspace.cpp
> No comments.
>
> src/share/vm/memory/metaspaceShared.cpp
> No comments.
>
> src/share/vm/memory/metaspaceShared.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: }
> Sorry, this free() call still bothers me and now I think I
> see why. This getter:
>
> L3652: JvmtiCachedClassFileData* InstanceKlass::get_cached_class_file() {
> L3653: if (MetaspaceShared::is_in_shared_space(_cached_class_file)) {
> L3654: // Ignore the archived class stream data
> L3655: return NULL;
> L3656: } else {
>
> shows that the _cached_class_file field can refer to a shared
> class and when it does the getter returns NULL. Yes, this is
> the deallocation code path and we should never get here for a
> shared instanceKlass. Perhaps we need an assert at the top of
> function:
>
> assert(!this->is_shared(), "should not be called for a shared class");
>
> Feel free to say that I'm being way too paranoid here. :-)
An assert here sounds good to me. I’ll add it.
>
> L3671: assert(this->is_shared(), "class is not shared");
> Perhaps: "class should be shared" instead.
Fixed.
>
> src/share/vm/oops/instanceKlass.hpp
> No comments.
>
> src/share/vm/utilities/debug.cpp
> No comments.
>
> src/share/vm/utilities/debug.hpp
> No comments.
>
>
> Thumbs up. Don't need to see a new webrev if you take any of my
> minor suggestions above. If you do add the new assert() to the
> deallocation code path, you'll need a new round of testing.
Will do.
Thanks!
Jiangli
>
> Dan
>
>
>>
>> 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