[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Tue May 19 16:58:43 UTC 2015
Thanks for the review, Serguei.
Updated webrev in place:
http://cr.openjdk.java.net/~vlivanov/8059340/webrev.01
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