RFR: 8078644: CDS needs to support JVMTI CFLH
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Sep 14 21:17:31 UTC 2016
Ok.
Thanks, Jiangli
Serguei
On 9/14/16 09:20, Jiangli Zhou wrote:
> Hi Serguei,
>
> Thank you for double checking each detail.
>
> The partition_size argument for ReservedSpace::last_part() is actually
> the size of the first partition. The base of the last_part is
> calculated as base()+partition_size. So at line 102, using
> core_space_size is correct.
>
>
> ReservedSpace ReservedSpace::first_part(size_t partition_size, size_t
> alignment,
> bool split, bool realloc) {
> assert(partition_size <= size(), "partition failed");
> if (split) {
> os::split_reserved_memory(base(), size(), partition_size, realloc);
> }
> ReservedSpace result(base(), partition_size, alignment, special(),
> executable());
> return result;
> }
>
> ReservedSpace
> ReservedSpace::last_part(size_t partition_size, size_t alignment) {
> assert(partition_size <= size(), "partition failed");
> ReservedSpace result(*base() + partition_size*, size() - partition_size,
> alignment, special(), executable());
> return result;
> }
>
> I actually wondered the same about the partition of shared_ro_rw &
> misc_section before looking into the ReservedSpace::last_part()
> implementation.
>
> Thanks,
>
> Jiangli
>
>> On Sep 13, 2016, at 10:22 PM, serguei.spitsyn at oracle.com
>> <mailto:serguei.spitsyn at oracle.com> wrote:
>>
>> Hi Jiangli,
>>
>>
>> http://cr.openjdk.java.net/~jiangli/8078644/webrev.02/src/share/vm/memory/metaspaceShared.cpp.frames.html
>>
>> 100 // Split into the core and optional sections
>> 101 ReservedSpace core_data = _shared_rs->first_part(core_spaces_size);
>> *102 ReservedSpace optional_data =
>> _shared_rs->last_part(core_spaces_size);*
>> 103
>> 104 // The RO/RW and the misc sections
>> 105 ReservedSpace shared_ro_rw = core_data.first_part(metadata_size);
>> 106 ReservedSpace misc_section = core_data.last_part(metadata_size);
>> 107
>> 108 // Now split the misc code and misc data sections.
>> 109 ReservedSpace md_rs = misc_section.first_part(SharedMiscDataSize);
>> 110 ReservedSpace mc_rs = misc_section.last_part(SharedMiscDataSize);
>> 111
>> 112 _md.initialize(md_rs, SharedMiscDataSize, SharedMiscData);
>> 113 _mc.initialize(mc_rs, SharedMiscCodeSize, SharedMiscCode);
>> *114 _od.initialize(optional_data, metadata_size, SharedOptional);*
>> Should the *optional_data* have the *core_space_size* at L114 match
>> the L102?
>> Would it be even better to useoptional_spaces_size() ? :
>> ??? size_t optional_spaces_size =
>> FileMapInfo::optional_spaces_size(); 102 ReservedSpace optional_data
>> = _shared_rs->last_part(optional_spaces_size);
>> 114 _od.initialize(optional_data, optional_spaces_size, SharedOptional);
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 9/13/16 11:02, 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/
>>>
>>> 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