[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Thu May 21 10:55:31 UTC 2015
Thanks, Coleen.
It shouldn't be a hassle to add CLD::_handles back back if needed.
Best regards,
Vladimir Ivanov
PS: have a good vacation!
On 5/21/15 12:27 AM, Coleen Phillimore wrote:
> I am on vacation and can't read this webrev from my iPhone. I assume that Stefan and Serguei's and others reviews are good. The CLD _handles field could be used to hold the mirrors in a later change and might be needed for jigsaw so might have to come back then.
> Thank you for doing this change!!
> Coleen
>
> Sent from my iPhone
>
>> On May 20, 2015, at 1:58 PM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>>
>> Thanks for spotting that, Stefan.
>>
>> Updated webrev in place:
>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.02
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>> On 5/20/15 8:48 PM, Stefan Karlsson wrote:
>>>> On 2015-05-20 17:09, Vladimir Ivanov wrote:
>>>> Stefan, Chris,
>>>>
>>>> Yes, you are right. ClassLoaderData::_handles isn't used anymore and
>>>> can go away.
>>>
>>> Great.
>>>>
>>>> Updated webrev:
>>>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.02
>>>
>>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.02/hotspot/src/share/vm/classfile/classLoaderData.cpp.udiff.html
>>>
>>>
>>> // release the handles
>>> - if (_handles != NULL) {
>>> - JNIHandleBlock::release_block(_handles);
>>> - _handles = NULL;
>>> - }
>>>
>>> The comment should be removed.
>>>
>>> Could this include be removed?
>>>
>>> 63 #include "runtime/jniHandles.hpp"
>>>
>>>
>>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.02/hotspot/src/share/vm/classfile/classLoaderData.hpp.frames.html
>>>
>>>
>>> 54 class JNIHandleBlock;
>>>
>>> The forward declaration could be removed.
>>>
>>> Thanks,
>>> StefanK
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>>> On 5/20/15 9:15 AM, Stefan Karlsson wrote:
>>>>> On 2015-05-20 03:54, Christian Thalinger wrote:
>>>>>>> On May 19, 2015, at 9:58 AM, Vladimir Ivanov
>>>>>>> <vladimir.x.ivanov at oracle.com> wrote:
>>>>>>>
>>>>>>> Thanks for the review, Serguei.
>>>>>>>
>>>>>>> Updated webrev in place:
>>>>>>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.01
>>>>>>> <http://cr.openjdk.java.net/~vlivanov/8059340/webrev.01>
>>>>>> Shouldn’t there be some GC code in InstanceKlass that can be removed
>>>>>> now?
>>>>>
>>>>> Yes, this is a nice patch from the GCs perspective, since it removes
>>>>> some of the work that we need to perform during the root processing.
>>>>>
>>>>> Unless I'm mistaken, you removed the only calls to
>>>>> ClassLoadeData::add_handle, so I think you should remove the handles
>>>>> block in ClassLoaderData.
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>>>
>>>>>> + private transient Object[] resolved_references;
>>>>>>
>>>>>> We should follow Java naming conventions and use “resolvedReferences”.
>>>>>>
>>>>>>> Switched to ConstantPool::resolved_references() as you suggested.
>>>>>>>
>>>>>>> Regarding declaring the field in vmStructs.cpp, it is not needed
>>>>>>> since the field is located in Java mirror and not in InstanceKlass.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>>
>>>>>>>> On 5/19/15 10:47 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Vladimir,
>>>>>>>>
>>>>>>>> It looks good in general.
>>>>>>>> Some comments are below.
>>>>>>>>
>>>>>>>> || *src/share/vm/oops/cpCache.cpp*
>>>>>>>>
>>>>>>>> @@ -281,11 +281,11 @@
>>>>>>>> // Competing writers must acquire exclusive access via a lock.
>>>>>>>> // A losing writer waits on the lock until the winner writes f1
>>>>>>>> and leaves
>>>>>>>> // the lock, so that when the losing writer returns, he can use
>>>>>>>> the linked
>>>>>>>> // cache entry.
>>>>>>>>
>>>>>>>> - objArrayHandle resolved_references = cpool->resolved_references();
>>>>>>>> + objArrayHandle resolved_references =
>>>>>>>> cpool->pool_holder()->resolved_references();
>>>>>>>> // Use the resolved_references() lock for this cpCache entry.
>>>>>>>> // resolved_references are created for all classes with
>>>>>>>> Invokedynamic, MethodHandle
>>>>>>>> // or MethodType constant pool cache entries.
>>>>>>>> assert(resolved_references() != NULL,
>>>>>>>> "a resolved_references array should have been created for
>>>>>>>> this class");
>>>>>>>>
>>>>>>>> ------------------------------------------------------------------------
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> @@ -410,20 +410,20 @@
>>>>>>>>
>>>>>>>> oop
>>>>>>>> ConstantPoolCacheEntry::appendix_if_resolved(constantPoolHandle
>>>>>>>> cpool) {
>>>>>>>> if (!has_appendix())
>>>>>>>> return NULL;
>>>>>>>> const int ref_index = f2_as_index() +
>>>>>>>> _indy_resolved_references_appendix_offset;
>>>>>>>> - objArrayOop resolved_references = cpool->resolved_references();
>>>>>>>> + objArrayOop resolved_references =
>>>>>>>> cpool->pool_holder()->resolved_references();
>>>>>>>> return resolved_references->obj_at(ref_index);
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> oop
>>>>>>>> ConstantPoolCacheEntry::method_type_if_resolved(constantPoolHandle
>>>>>>>> cpool) {
>>>>>>>> if (!has_method_type())
>>>>>>>> return NULL;
>>>>>>>> const int ref_index = f2_as_index() +
>>>>>>>> _indy_resolved_references_method_type_offset;
>>>>>>>> - objArrayOop resolved_references = cpool->resolved_references();
>>>>>>>> + objArrayOop resolved_references =
>>>>>>>> cpool->pool_holder()->resolved_references();
>>>>>>>> return resolved_references->obj_at(ref_index);
>>>>>>>> }
>>>>>>>>
>>>>>>>> There is no need in the update above as the constant pool still
>>>>>>>> has the
>>>>>>>> function resolved_references():
>>>>>>>> +objArrayOop ConstantPool::resolved_references() const {
>>>>>>>> + return pool_holder()->resolved_references();
>>>>>>>> +}
>>>>>>>>
>>>>>>>> The same is true for the files:
>>>>>>>> src/share/vm/interpreter/interpreterRuntime.cpp
>>>>>>>> src/share/vm/interpreter/bytecodeTracer.cpp
>>>>>>>> || src/share/vm/ci/ciEnv.cpp
>>>>>>>>
>>>>>>>>
>>>>>>>> || src/share/vm/runtime/vmStructs.cpp*
>>>>>>>>
>>>>>>>> *@@ -286,11 +286,10 @@
>>>>>>>> nonstatic_field(ConstantPool, _tags,
>>>>>>>> Array<u1>*) \
>>>>>>>> nonstatic_field(ConstantPool, _cache,
>>>>>>>> ConstantPoolCache*) \
>>>>>>>> nonstatic_field(ConstantPool, _pool_holder,
>>>>>>>> InstanceKlass*) \
>>>>>>>> nonstatic_field(ConstantPool, _operands,
>>>>>>>> Array<u2>*) \
>>>>>>>> nonstatic_field(ConstantPool, _length,
>>>>>>>> int) \
>>>>>>>> - nonstatic_field(ConstantPool, _resolved_references,
>>>>>>>> jobject) \
>>>>>>>> nonstatic_field(ConstantPool, _reference_map,
>>>>>>>> Array<u2>*) \
>>>>>>>> nonstatic_field(ConstantPoolCache, _length,
>>>>>>>> int) \
>>>>>>>> nonstatic_field(ConstantPoolCache, _constant_pool,
>>>>>>>> ConstantPool*) \
>>>>>>>> nonstatic_field(InstanceKlass, _array_klasses,
>>>>>>>> Klass*) \
>>>>>>>> nonstatic_field(InstanceKlass, _methods,
>>>>>>>> Array<Method*>*) \
>>>>>>>> *
>>>>>>>>
>>>>>>>> *I guess, we need to cover the same field in the InstanceKlass
>>>>>>>> instead.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 5/18/15 10:32 AM, Vladimir Ivanov wrote:
>>>>>>>>> Here's updated version:
>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.01
>>>>>>>>>
>>>>>>>>> Moved ConstantPool::_resolved_references to mirror class instance.
>>>>>>>>>
>>>>>>>>> Fixed a couple of issues in CDS and JVMTI (class redefinition)
>>>>>>>>> caused
>>>>>>>>> by this change.
>>>>>>>>>
>>>>>>>>> I had to hard code Class::resolved_references offset since it is
>>>>>>>>> used
>>>>>>>>> in template interpreter which is generated earlier than j.l.Class is
>>>>>>>>> loaded during VM bootstrap.
>>>>>>>>>
>>>>>>>>> Testing: hotspot/test, vm testbase (in progress)
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Vladimir Ivanov
>>>>>>>>>
>>>>>>>>>> On 4/21/15 9:30 PM, Vladimir Ivanov wrote:
>>>>>>>>>> Coleen, Chris,
>>>>>>>>>>
>>>>>>>>>> I'll proceed with moving ConstantPool::_resolved_references to
>>>>>>>>>> j.l.Class
>>>>>>>>>> instance then.
>>>>>>>>>>
>>>>>>>>>> Thanks for the feedback.
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> Vladimir Ivanov
>>>>>>>>>>
>>>>>>>>>> On 4/21/15 3:22 AM, Christian Thalinger wrote:
>>>>>>>>>>>> On Apr 20, 2015, at 4:34 PM, Coleen Phillimore
>>>>>>>>>>>> <coleen.phillimore at oracle.com
>>>>>>>>>>>> <mailto:coleen.phillimore at oracle.com>>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Vladimir,
>>>>>>>>>>>>
>>>>>>>>>>>> I think that changing the format of the heap dump isn't a good
>>>>>>>>>>>> idea
>>>>>>>>>>>> either.
>>>>>>>>>>>>
>>>>>>>>>>>>> On 4/16/15, 2:12 PM, Vladimir Ivanov wrote:
>>>>>>>>>>>>> (sorry for really late response; just got enough time to
>>>>>>>>>>>>> return to
>>>>>>>>>>>>> the bug)
>>>>>>>>>>>> I'd forgotten about it!
>>>>>>>>>>>>> Coleen, Staffan,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks a lot for the feedback!
>>>>>>>>>>>>>
>>>>>>>>>>>>> After thinking about the fix more, I don't think that using
>>>>>>>>>>>>> reserved
>>>>>>>>>>>>> oop slot in CLASS DUMP for recording _resolved_references is
>>>>>>>>>>>>> the best
>>>>>>>>>>>>> thing to do. IMO the change causes too much work for the users
>>>>>>>>>>>>> (heap
>>>>>>>>>>>>> dump analysis tools).
>>>>>>>>>>>>>
>>>>>>>>>>>>> It needs specification update and then heap dump analyzers
>>>>>>>>>>>>> should be
>>>>>>>>>>>>> updated as well.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have 2 alternative approaches (hacky and not-so-hacky :-)):
>>>>>>>>>>>>>
>>>>>>>>>>>>> - artificial class static field in the dump
>>>>>>>>>>>>> ("<resolved_references>"
>>>>>>>>>>>>> + optional id to guarantee unique name);
>>>>>>>>>>>>>
>>>>>>>>>>>>> - add j.l.Class::_resolved_references field;
>>>>>>>>>>>>> Not sure how much overhead (mostly reads from bytecode) the
>>>>>>>>>>>>> move
>>>>>>>>>>>>> from ConstantPool to j.l.Class adds, so I propose just to
>>>>>>>>>>>>> duplicate
>>>>>>>>>>>>> it for now.
>>>>>>>>>>>> I really like this second approach, so much so that I had a
>>>>>>>>>>>> prototype
>>>>>>>>>>>> for moving resolved_references directly to the j.l.Class object
>>>>>>>>>>>> about
>>>>>>>>>>>> a year ago. I couldn't find any benefit other than
>>>>>>>>>>>> consolidating oops
>>>>>>>>>>>> so the GC would have less work to do. If the
>>>>>>>>>>>> resolved_references are
>>>>>>>>>>>> moved to j.l.C instance, they can not be jobjects and the
>>>>>>>>>>>> ClassLoaderData::_handles area wouldn't have to contain them (but
>>>>>>>>>>>> there are other things that could go there so don't delete the
>>>>>>>>>>>> _handles field yet).
>>>>>>>>>>>>
>>>>>>>>>>>> The change I had was relatively simple. The only annoying
>>>>>>>>>>>> part was
>>>>>>>>>>>> that getting to the resolved references has to be in
>>>>>>>>>>>> macroAssembler
>>>>>>>>>>>> and do:
>>>>>>>>>>>>
>>>>>>>>>>>> go through
>>>>>>>>>>>> method->cpCache->constants->instanceKlass->java_mirror()
>>>>>>>>>>>> rather than
>>>>>>>>>>>> method->cpCache->constants->resolved_references->jmethod
>>>>>>>>>>>> indirection
>>>>>>>>>>>>
>>>>>>>>>>>> I think it only affects the interpreter so the extra indirection
>>>>>>>>>>>> wouldn't affect performance, so don't duplicate it! You don't
>>>>>>>>>>>> want to
>>>>>>>>>>>> increase space used by j.l.C without taking it out somewhere
>>>>>>>>>>>> else!
>>>>>>>>>>> I like this approach. Can we do this?
>>>>>>>>>>>
>>>>>>>>>>>>> What do you think about that?
>>>>>>>>>>>> Is this bug worth doing this? I don't know but I'd really
>>>>>>>>>>>> like it.
>>>>>>>>>>>>
>>>>>>>>>>>> Coleen
>>>>>>>>>>>>
>>>>>>>>>>>>> Best regards,
>>>>>>>>>>>>> Vladimir Ivanov
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 10/6/14 11:35 AM, Staffan Larsen wrote:
>>>>>>>>>>>>>> This looks like a good approach. However, there are a couple
>>>>>>>>>>>>>> of more
>>>>>>>>>>>>>> places that need to be updated.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The hprof binary format is described in
>>>>>>>>>>>>>> jdk/src/jdk.hprof.agent/share/native/libhprof/manual.html and
>>>>>>>>>>>>>> needs
>>>>>>>>>>>>>> to be updated. It’s also more formally specified in
>>>>>>>>>>>>>> hprof_b_spec.h
>>>>>>>>>>>>>> in the same directory.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The hprof JVMTI agent in jdk/src/jdk.hprof.agent code would
>>>>>>>>>>>>>> also
>>>>>>>>>>>>>> need to be updated to show this field. Since this is a JVMTI
>>>>>>>>>>>>>> agent
>>>>>>>>>>>>>> it needs to be possible to find the resolved_refrences array
>>>>>>>>>>>>>> via the
>>>>>>>>>>>>>> JVMTI heap walking API. Perhaps that already works? - I haven’t
>>>>>>>>>>>>>> looked.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Finally, the Serviceability Agent implements yet another hprof
>>>>>>>>>>>>>> binary dumper in
>>>>>>>>>>>>>> hotspot/agent//src/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> which also needs to write this reference.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> /Staffan
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 29 sep 2014, at 16:51, Vladimir Ivanov
>>>>>>>>>>>>>> <vladimir.x.ivanov at oracle.com
>>>>>>>>>>>>>> <mailto:vladimir.x.ivanov at oracle.com>>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.00/
>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8059340
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> VM heap dump doesn't contain
>>>>>>>>>>>>>>> ConstantPool::_resolved_references for
>>>>>>>>>>>>>>> classes which have resolved references.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ConstantPool::_resolved_references points to an Object[]
>>>>>>>>>>>>>>> holding
>>>>>>>>>>>>>>> resolved constant pool entries (patches for VM anonymous
>>>>>>>>>>>>>>> classes,
>>>>>>>>>>>>>>> linked CallSite & MethodType for invokedynamic instructions).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I've decided to use reserved slot in HPROF class header
>>>>>>>>>>>>>>> format.
>>>>>>>>>>>>>>> It requires an update in jhat to correctly display new info.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The other approach I tried was to dump the reference as a fake
>>>>>>>>>>>>>>> static field [1], but storing VM internal
>>>>>>>>>>>>>>> ConstantPool::_resolved_references among user defined fields
>>>>>>>>>>>>>>> looks
>>>>>>>>>>>>>>> confusing.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Testing: manual (verified that corresponding arrays are
>>>>>>>>>>>>>>> properly
>>>>>>>>>>>>>>> linked in Nashorn heap dump).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Best regards,
>>>>>>>>>>>>>>> Vladimir Ivanov
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [1] http://cr.openjdk.java.net/~vlivanov/8059340/static
>>>
More information about the hotspot-dev
mailing list