[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Wed May 20 13:06:44 UTC 2015
Thanks for review, Chris.
>> Updated webrev in place:
>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.01
>
> Shouldn’t there be some GC code in InstanceKlass that can be removed now?
I don't think so. It was a handle, so no special GC logic was needed.
And now it is a raw oop.
> + private transient Object[] resolved_references;
>
> We should follow Java naming conventions and use “resolvedReferences”.
Good point. Will fix as you suggest.
Best regards,
Vladimir Ivanov
>
>>
>> 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
>> <mailto: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