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

List:       openjdk-serviceability-dev
Subject:    Re: RFR(M, v12): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
From:       Dmitry Samersoff <dmitry.samersoff () oracle ! com>
Date:       2015-06-03 8:48:11
Message-ID: 556EBF4B.6070400 () oracle ! com
[Download RAW message or body]

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@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.


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