[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 19:02:04 UTC 2015
Thank you for the update!
You are right about the vmStructs.cpp.
One more minor comment:
InterpreterMacroAssembler::get_resolved_references for aarch64 is
defined in the .hpp
but for other architectures it is in the .cpp which is inconsistent.
But I leave it up to you, consider it reviewed.
Now I wonder if we have to merge the resolved references array
when a class is redefined and the old version is still in use.
I'll check if there is already a bug covering this potential issue.
Thanks,
Serguei
On 5/19/15 9:58 AM, Vladimir Ivanov wrote:
> 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