[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(M, v11): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
From:       Mikael Gerdin <mikael.gerdin () oracle ! com>
Date:       2015-06-03 9:14:21
Message-ID: 556EC56D.3080703 () oracle ! com
[Download RAW message or body]



On 2015-06-02 23:04, Dmitry Samersoff wrote:
> Mikael,
>
> The reason of placing get_filed_offset_by_name to the oop.inline is that
> hotspot widely duplicate this code.
>
> Symbol is used to identify a field within klass, not a klass them self
> so I think it's OK to have it tied to the oopDesc.

Ok, I didn't relize that both the symbols were used for the field, sorry 
about that.
I still strongly feel that the method should be on InstanceKlass though.
/Mikael

>
> -Dmitry
>
> On 2015-06-02 15:06, Mikael Gerdin wrote:
>> Hi Dmitry,
>>
>> On 2015-06-02 13:12, Dmitry Samersoff wrote:
>>> Staffan,
>>>
>>>> Instead of hardcoding the field offsets, you can use
>>>> InstanceKlass::find_field and fieldDescriptor::offset to find the
>>>> offset at runtime.
>>>
>>> Done. Please, see
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.11
>>>
>>> I put the function int get_filed_offset_by_name(Symbol*,Symbol*) to
>>> oop.inline.hpp leaving a room for further cleanup because I found couple
>>> of places in hotspot that implements mostly similar functionality.
>>
>> Sorry for this sort of drive-by review, but I really don't think
>> get_field_offset_by_name should be in the oop class. If anywhere it
>> should be on InstanceKlass, and using Symbol* to identify a Klass* seems
>> incorrect since the same symbol can refer to different classes in
>> different class loader contexts.
>>
>> /Mikael
>>
>>>
>>> -Dmitry
>>>
>>> On 2015-06-01 10:18, Staffan Larsen wrote:
>>>> Dmitry,
>>>>
>>>> Instead of hardcoding the field offsets, you can use
>>>> InstanceKlass::find_field and fieldDescriptor::offset to find the
>>>> offset at runtime.
>>>>
>>>> Thanks,
>>>> /Staffan
>>>>
>>>>> On 31 maj 2015, at 13:43, Dmitry Samersoff
>>>>> <dmitry.samersoff@oracle.com> wrote:
>>>>>
>>>>> Everyone,
>>>>>
>>>>> Please take a look at new version of the fix.
>>>>>
>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.10/
>>>>>
>>>>> Changes (to previous version) are in
>>>>> Finalizer.java and diagnosticCommand.cpp
>>>>>
>>>>> This version copy data from Map.Entry<> to array of
>>>>> FinalizerHistogramEntry instances then,
>>>>> VM prints content of this array.
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2015-05-28 21:06, Mandy Chung wrote:
>>>>>>
>>>>>> On 05/28/2015 07:35 AM, Peter Levart wrote:
>>>>>>> Hi Mandy,
>>>>>>>
>>>>>>> On 05/27/2015 03:32 PM, Mandy Chung wrote:
>>>>>>>> Taking it further - is it simpler to return String[] of all
>>>>>>>> classnames including the duplicated ones and have the VM do the
>>>>>>>> count?  Are you concerned with the size of the String[]?
>>>>>>>
>>>>>>> Yes, the histogram is much smaller than the list of all instances.
>>>>>>> There can be millions of instances waiting in finalizer queue, but
>>>>>>> only a few distinct classes.
>>>>>>
>>>>>> Do you happen to know what libraries are the major contributors to
>>>>>> these
>>>>>> millions of finalizers?
>>>>>>
>>>>>> It has been strongly recommended to avoid finalizers (see Effective
>>>>>> Java
>>>>>> Item 7).   I'm not surprised that existing code is still using
>>>>>> finalizers while we should really encourage them to migrate away
>>>>>> from it.
>>>>>>
>>>>>>> What could be done in Java to simplify things in native code but
>>>>>>> still
>>>>>>> not format the output is to convert the array of Map.Entry(s) into an
>>>>>>> Object[] array of alternating {String, int[], String, int[], .... }
>>>>>>>
>>>>>>> Would this simplify native code for the price of a little extra work
>>>>>>> in Java? The sizes of old and new arrays are not big (# of distinct
>>>>>>> classes of finalizable objects in the queue).
>>>>>>
>>>>>> I also prefer writing in Java and writing less native code (much
>>>>>> simplified).  It's an interface that we have to agree upon and keep it
>>>>>> simple.  Having the returned Object[] as alternate String and int[]
>>>>>> is a
>>>>>> reasonable compromise.
>>>>>>
>>>>>> ReferenceQueue.java - you can move @SuppressWarning from the method to
>>>>>> just the field variable "rn"
>>>>>>       @SuppressWarnings("unchecked")
>>>>>>       Reference<? extends T> rn = r.next;
>>>>>>
>>>>>> Finalizer.java
>>>>>>       It's better to rename printFinalizationQueue as it inspects the
>>>>>> finalizer reference queue (maybe getFinalizerHistogram?).  Can you
>>>>>> move
>>>>>> this method to the end of this class and add the comment saying that
>>>>>> this is invoked by VM for jcmd -finalizerinfo support and @return to
>>>>>> describe the returned content.  I also think you can remove
>>>>>> @SuppressWarnings for this method.
>>>>>>
>>>>>> Mandy
>>>>>
>>>>>
>>>>> --
>>>>> Dmitry Samersoff
>>>>> Oracle Java development team, Saint Petersburg, Russia
>>>>> * I would love to change the world, but they won't give me the sources.
>>>>
>>>
>>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]