[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Wed May 20 17:58:13 UTC 2015
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