[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed May 20 23:25:51 UTC 2015
Looks good.
Thanks,
Serguei
On 5/20/15 10:58 AM, Vladimir Ivanov 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