[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