RFR(M, v13): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Everyone,

Updated webrev:

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13

Changes to oop.inline.hpp is reverted and find_field used directly is
diagnostic command.

-Dmitry

On 2015-06-03 11:48, Dmitry Samersoff wrote:
> Everyone,
> 
> Updated webrev:
> 
> http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.12
> 
> getFinalizerHistogram and FinalizerHistogramEntry moved to separate
> package-private class. Hotspot code changed accordingly.
> 
> getFinalizerHistogram updated to use Peter's code.
> 
> -Dmitry
> 
> 
> On 2015-06-03 09:06, Peter Levart wrote:
>> Hi Dmitry,
>>
>> On 06/02/2015 01:12 PM, 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
>>
>> In the jdk part, now that you have a FinalizerHistogramEntry class, you
>> can simplify the code even further:
>>
>>
>>     private static final class FinalizerHistogramEntry {
>>         int instanceCount;
>>         final String className;
>>
>>         int getInstanceCount() {
>>             return instanceCount;
>>         }
>>
>>         FinalizerHistogramEntry(String className) {
>>             this.className = className;
>>         }
>>     }
>>
>>     static Object[] getFinalizerHistogram() {
>>         Map<String, FinalizerHistogramEntry> countMap = new HashMap<>();
>>         queue.forEach(r -> {
>>             Object referent = r.get();
>>             if (referent != null) {
>>                 countMap.computeIfAbsent(
>>                         referent.getClass().getName(),
>>                         FinalizerHistogramEntry::new).instanceCount++;
>>                 /* Clear stack slot containing this variable, to decrease
>>                    the chances of false retention with a conservative GC */
>>                 referent = null;
>>             }
>>         });
>>
>>         FinalizerHistogramEntry fhe[] = countMap.values()
>>                 .toArray(new FinalizerHistogramEntry[countMap.size()]);
>>         Arrays.sort(fhe,
>>                 Comparator.comparingInt(
>>                        
>> FinalizerHistogramEntry::getInstanceCount).reversed());
>>         return fhe;
>>     }
>>
>>
>> ... see, no copying loop required. Also notice the access modifier used
>> on the nested class and it's fields/method/constructor. This way the
>> class is private and fields can be accessed from getFinalizerHistogram
>> and lambda without compiler generating access bridges.
>>
>>
>> Regards, Peter
>>
>>>
>>> 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.
>>>
>>> -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 at 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.
>>>
>>
> 
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.