[9] RFR (S): 8059340: ConstantPool::_resolved_references is missing in heap dump
Stefan Karlsson
stefan.karlsson at oracle.com
Wed May 20 06:15:53 UTC 2015
On 2015-05-20 03:54, Christian Thalinger wrote:
>> On May 19, 2015, at 9:58 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>>
>> Thanks for the review, Serguei.
>>
>> Updated webrev in place:
>> http://cr.openjdk.java.net/~vlivanov/8059340/webrev.01 <http://cr.openjdk.java.net/~vlivanov/8059340/webrev.01>
> Shouldn’t there be some GC code in InstanceKlass that can be removed now?
Yes, this is a nice patch from the GCs perspective, since it removes
some of the work that we need to perform during the root processing.
Unless I'm mistaken, you removed the only calls to
ClassLoadeData::add_handle, so I think you should remove the handles
block in ClassLoaderData.
Thanks,
StefanK
>
> + private transient Object[] resolved_references;
>
> We should follow Java naming conventions and use “resolvedReferences”.
>
>> 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