[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
Stefan Karlsson
stefan.karlsson at oracle.com
Wed May 20 17:48:27 UTC 2015
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