Hi Serguei, On Fri, Jun 22, 2018 at 11:55 PM, serguei.spit...@oracle.com <serguei.spit...@oracle.com> wrote: > Hi Thomas, > > > It looks good to me. > A couple of minor comments below. > > http://cr.openjdk.java.net/~stuefe/webrevs/8205531-vm.classloader-tree-folding/webrev.00/webrev/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html > > 135 // we we add a preliminary empty LoaderTreeNode for it. This > preliminary node > > A typo: "we we" > > 151 // In default view, similar tree nodes (same loader class, same or no > name) > > I'd suggest a minor tweak: "same or no name" => "same name or no name" as > at the line: > > 291 // and they have the same name or no name (note: leaf check is > done by caller). > > Otherwise, it is a little bit confusing because of the preceding "same > loader class". > >
Makes all sense. Update: Delta: http://cr.openjdk.java.net/~stuefe/webrevs/8205531-vm.classloader-tree-folding/webrev.00-to-01/webrev/ Full: http://cr.openjdk.java.net/~stuefe/webrevs/8205531-vm.classloader-tree-folding/webrev.01/webrev/ I only changed the comments as suggested. Tests (jdk-submit) came back fine. > > On 6/22/18 12:10, Thomas Stüfe wrote: > > Resent with the correct subject, sorry. > > > Also, it is better to list a bug number in the email subject. :) > :) I really must have been tired yesterday. But I will not change the subject again, to not tear up the thread. Thanks, Thomas > Thanks, > Serguei > > ..Thomas > > On Fri, Jun 22, 2018 at 9:08 PM, Thomas Stüfe <thomas.stu...@gmail.com> > wrote: > > Hi all, > > may I have reviews for this small enhancement to the jcmd > VM.classloader diagnostic command: > > https://bugs.openjdk.java.net/browse/JDK-8205531 > http://cr.openjdk.java.net/~stuefe/webrevs/8205531-vm.classloader-tree-folding/webrev.00/webrev/ > > > VM.classloaders prints a tree of class loaders. This tree can grow a > lot and become unwieldy, especially with a lot of similar loaders. One > prime example is the DelegatingClassLoader. It would be helpful were > all these loaders: > > 13114: > +-- <bootstrap> > | > +-- "platform", jdk.internal.loader.ClassLoaders$PlatformClassLoader > | > +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader > | > +-- test3.internals.InMemoryClassLoader > | > +-- jdk.internal.reflect.DelegatingClassLoader > | > +-- jdk.internal.reflect.DelegatingClassLoader > | > +-- jdk.internal.reflect.DelegatingClassLoader > | > +-- jdk.internal.reflect.DelegatingClassLoader > | > +-- jdk.internal.reflect.DelegatingClassLoader > | > ...... repeat 1495 times > > folded into one: > > 13114: > +-- <bootstrap> > | > +-- "platform", jdk.internal.loader.ClassLoaders$PlatformClassLoader > | > +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader > | > +-- test3.internals.InMemoryClassLoader > | > +-- jdk.internal.reflect.DelegatingClassLoader > (+ 1499 more) > > > Original idea by Bernd Eckenfels, see > http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023824.html > > Thank you, Thomas > >
