[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Wed May 20 15:09:04 UTC 2015
Stefan, Chris,
Yes, you are right. ClassLoaderData::_handles isn't used anymore and can
go away.
Updated webrev:
http://cr.openjdk.java.net/~vlivanov/8059340/webrev.02
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