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



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. :)

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

Reply via email to