[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(M, v13): JDK-8059036 : Implement Diagnostic Commands for heap and
From: Mandy Chung <mandy.chung () oracle ! com>
Date: 2015-06-03 22:56:34
Message-ID: 523E93C8-B707-4CF6-A26E-B5400E04C402 () oracle ! com
[Download RAW message or body]
> On Jun 3, 2015, at 12:22 PM, Peter Levart <peter.levart@gmail.com> wrote:
>
>
> On 06/03/2015 08:36 PM, Mandy Chung wrote:
> >
> >
> > On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
> > > Updated webrev:
> > >
> > > http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13
> >
> > I reviewed the jdk change. The version looks okay and some comments:
> >
> > ReferenceQueue.java - you should eliminate the use of rawtype:
> >
> > 84 Reference rn = r.next;
> >
> > Change Reference to Reference<? extends T>
> >
> > The forEach method - eliminate the use of raw type and
> > move @SuppressWarning to the field variable in line 182:
> >
> > @SuppressWarnings("unchecked")
> > Reference<? extends T> rn = r.next;
>
> Thanks, Mandy. This was my doing. The @SuppressWarnings can be moved to the local \
> var declaration in line 84 too. Here's how the fixed ReferenceQueue looks like \
> after those two changes:
> http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.03/
>
This looks good.
>
> >
> > FinalizerHistogram.java
> > Copyright year needs update.
> >
> > I personally think the VM code would be much simpler if you stay with alternative \
> > entry of String and int than dealing with a FinalizerHistogramEntry private \
> > class. It's okay as FinalizerHistogram class is separated.
> > The comment in line 35 is suitable to move to the class description and this is \
> > the suggestion.
> > // This FinalizerHistogram class is for GC.finalizer_info diagnostic command \
> > support. // It is invoked by the VM.
> >
> > Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM knows the \
> > returned type anyway. It's an inner class of FinalizerHistogram and it can \
> > simply be named as "Entry". I suggest to have Entry::increment method to \
> > replace ".instanceCount++".
>
> This FinalizerHistogram.Entry naming part came to my mind too, yes. If there is a \
> method for incrementing, then both fields can be marked private.
Yup.
Mandy
>
> Regards, Peter
>
> >
> > The tests are in hotspot/test. Can you add a unit test in jdk/test? Perhaps \
> > you can test FinalizerHistogram.getFinalizerHistogram() via reflection - just a \
> > sanity test.
> > A minor naming comment - now you have a FinalizerHistogram class. Perhaps the \
> > getFinalizerHistogram method should be renamed e.g. "dump"?
> > Mandy
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic