[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
From: Dmitry Samersoff <dmitry.samersoff () oracle ! com>
Date: 2015-06-08 11:05:27
Message-ID: 557576F7.4040000 () oracle ! com
[Download RAW message or body]
Staffan,
Could you review a CCC request.
http://ccc.us.oracle.com/8059036
-Dmitry
On 2015-06-05 15:24, Staffan Larsen wrote:
> Thanks - this version looks good to me.
>
> /Staffan
>
> > On 5 jun 2015, at 13:00, Dmitry Samersoff <dmitry.samersoff@oracle.com> wrote:
> >
> > Staffan,
> >
> > Thank you for review!
> >
> > Done. Webrev updated in-place (press shift-reload).
> >
> > http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
> >
> > -Dmitry
> >
> > On 2015-06-05 11:20, Staffan Larsen wrote:
> > > Dmitry,
> > >
> > > I’d like to propose the following change to get prettier output (more in line \
> > > with GC.class_histogram):
> > > diff --git a/src/share/vm/services/diagnosticCommand.cpp \
> > > b/src/share/vm/services/diagnosticCommand.cpp
> > > --- a/src/share/vm/services/diagnosticCommand.cpp
> > > +++ b/src/share/vm/services/diagnosticCommand.cpp
> > > @@ -341,7 +341,6 @@
> > > void FinalizerInfoDCmd::execute(DCmdSource source, TRAPS) {
> > > ResourceMark rm;
> > >
> > > - output()->print_cr("Unreachable instances awaiting finalization:");
> > > Klass* k = SystemDictionary::resolve_or_null(
> > > vmSymbols::finalizer_histogram_klass(), THREAD);
> > > assert(k != NULL, "FinalizerHistogram class is not accessible");
> > > @@ -375,12 +374,15 @@
> > >
> > > assert(count_res != NULL && name_res != NULL, "Unexpected layout of \
> > > FinalizerHistogramEntry");
> > > + output()->print_cr("Unreachable instances waiting for finalization");
> > > + output()->print_cr("#instances class name");
> > > + output()->print_cr("-----------------------");
> > > for (int i = 0; i < result_oop->length(); ++i) {
> > > oop element_oop = result_oop->obj_at(i);
> > > oop str_oop = element_oop->obj_field(name_fd.offset());
> > > char *name = java_lang_String::as_utf8_string(str_oop);
> > > int count = element_oop->int_field(count_fd.offset());
> > > - output()->print_cr("Class %s - %d", name, count);
> > > + output()->print_cr("%10d %s", count, name);
> > > }
> > > }
> > >
> > >
> > > A couple of nits:
> > >
> > > diagnosticCommand.cpp:359 - extra space after =
> > > diagnosticCommand.cpp:361 - spelling: finlalization -> finalization
> > > FinalizerInfoTest.java:69: - spelling: intialized -> initialized
> > > FinalizerInfoTest.java:71 - I’d like to see the ; on a new line
> > >
> > >
> > > Thanks,
> > > /Staffan
> > >
> > >
> > > > On 5 jun 2015, at 00:20, Mandy Chung <mandy.chung@oracle.com> wrote:
> > > >
> > > >
> > > > > On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff <dmitry.samersoff@oracle.com> \
> > > > > wrote:
> > > > > Mandy,
> > > > >
> > > > > Webrev updated in-place (press shift-reload).
> > > > >
> > > > > http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
> > > > >
> > > > > getFinalizerHistogram() now returns Entry[]
> > > > >
> > > > > @library and @build removed from the test.
> > > >
> > > >
> > > > Looks fine.
> > > >
> > > > Thanks
> > > > Mandy
> > > >
> > > > >
> > > > > -Dmitry
> > > > >
> > > > > On 2015-06-04 16:56, Mandy Chung wrote:
> > > > > >
> > > > > > > On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff \
> > > > > > > <dmitry.samersoff@oracle.com> wrote:
> > > > > > > Mandy,
> > > > > > >
> > > > > > > Thank you for the review.
> > > > > > >
> > > > > > > Updated webrev is:
> > > > > > >
> > > > > > > http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
> > > > > > >
> > > > > >
> > > > > > Thanks for the update and the test.
> > > > > >
> > > > > > > addressed comments, added a unit test to JDK workspace.
> > > > > > >
> > > > > >
> > > > > > This test does not need @library and @build, right?
> > > > > >
> > > > > > >
> > > > > > > On 2015-06-03 21:36, Mandy Chung wrote:
> > > > > > >
> > > > > > > > Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
> > > > > > > > The VM knows the returned type anyway.
> > > > > > >
> > > > > > > "[java/lang/ref/Entry" signature would need a separate symbol entry in
> > > > > > > swollen vmSymbols::init() so I would prefer to stay with more generic
> > > > > > > Object[]
> > > > > > >
> > > > > >
> > > > > > You already add several symbols - why is it an issue for another one? Or \
> > > > > > if adding vm symbols is a concern, you should revert to String and int[] \
> > > > > > that you decide not to. The decision should apply consistently (use \
> > > > > > String and int, or new Java type).
> > > > > >
> > > > > > > > Perhaps the getFinalizerHistogram method should be renamed
> > > > > > > > e.g. "dump"?
> > > > > > >
> > > > > > > The line in vmSymbols looks like:
> > > > > > >
> > > > > > > template(
> > > > > > > get_finalizer_histogram_name, "getFinalizerHistogram")
> > > > > > >
> > > > > > > I would prefer to keep method name specific enough to be able to
> > > > > > > find it by grep in jdk code.
> > > > > >
> > > > > >
> > > > > > Grep in jdk code is easy as you have a new class :)
> > > > > >
> > > > > > Mandy
> > > > > >
> > > > > > >
> > > > > > > (other comments are addressed)
> > > > > > >
> > > > > > > -Dmitry
> > > > > > >
> > > > > > >
> > > > > > > On 2015-06-03 21:36, 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>
> > > > > > >
> > > > > > > done.
> > > > > > >
> > > > > > > >
> > > > > > > > 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;
> > > > > > >
> > > > > > > done.
> > > > > > >
> > > > > > > >
> > > > > > > > FinalizerHistogram.java
> > > > > > > > Copyright year needs update.
> > > > > > >
> > > > > > > done.
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > done.
> > > > > > >
> > > > > > > >
> > > > > > > > Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The \
> > > > > > > > VM knows the returned type anyway.
> > > > > > >
> > > > > > > "[java/lang/ref/Entry" signature would need a separate symbol entry in
> > > > > > > swollen vmSymbols::init() so I would prefer to stay with more generic
> > > > > > > Object[]
> > > > > > >
> > > > > > > > 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++".
> > > > > > >
> > > > > > > done.
> > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > done. The test repeats hotspot one, but uses reflection instead of VM \
> > > > > > > call.
> > > > > > > >
> > > > > > > > A minor naming comment - now you have a FinalizerHistogram class.
> > > > > > > > Perhaps the getFinalizerHistogram method should be renamed e.g. \
> > > > > > > > "dump"?
> > > > > > >
> > > > > > > The line in vmSymbols looks like:
> > > > > > >
> > > > > > > template(
> > > > > > > get_finalizer_histogram_name, "getFinalizerHistogram")
> > > > > > >
> > > > > > > I would prefer to keep it specific enough to be able to
> > > > > > > find it by grep in jdk code.
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > 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.
> > > >
> > >
> >
> >
> > --
> > 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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic