[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue May 19 07:47:36 UTC 2015
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