Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
Thanks Staffan! And I really appreciate
  all your reviews. Also thanks to Mikael for the DCMD test review.
  
  Cheers,
  
  Chris
  
  On 2/12/15 1:19 AM, Staffan Larsen wrote:


  
  Looks good! 
  
  
  Thanks for providing incremental webrevs - very
helpful!
  
  
  
  
  Thanks,
  /Staffan
  
  
  
  
  
  
  

  On 12 feb 2015, at 09:31, Chris Plummer 
wrote:
  
  


  I suppose it would be nice if
I included links to the webrevs:

http://cr.openjdk.java.net/~cjplummer/8054888/webrev.02-03/
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.03/

First one is just the latest changes described in the
previous email. 2nd one includes all changes.

thanks,

Chris

On 2/12/15 12:28 AM, Chris Plummer wrote:
  
  

Ok, hopefully the last
  webrev. :)
  
  -JPRT found a compiler error on Solaris.
  
  classfile/systemDictionary.hpp needed to be included
  due to the reference to
  
  SystemDictionary::Object_klass(). I assume this turned
  up on Solaris because it doesn't use precompiled
  headers.
  
  -I noticed I had inadvertently removed a ResourceMark
  in KlassInfoHisto::print_class_stats(). I think maybe
  I had done a cut-n-paste rather than copy-n-paste.
  This was in webrev.01 but was pretty inconspicuous in
  the webrev so went noticed.
  
  -I changed another WARNING to ERROR as Staffan
  requested.
  
  -I updated to the latest JDK9 sources and made the
  needed changes to the DCMD test as Mikael requested.
  
  thanks,
  
  Chris
  
  On 2/11/15 9:38 AM, Chris Plummer wrote:

On 2/11/15 2:12 AM, Mikael Auno
  wrote: 
  On 2015-02-11 04:13,
Chris Plummer wrote: 

  In general I
think this looks very good. Simple and
well-commented 
code to follow. I am missing a test, though.
Please look at the 
hotspot/test/serviceability/dcmd set of tests. 
  
  Added. 

Your test is based on DcmdUtil.java which was
removed last week (see 
[0]). If you pull new changes from hs-rt/hotspot,
you'll see plenty of 
tests using the new DCMD utility classes in
testlibrary. Also, the new 
tests are divided in different subdirectories
depending on the commands, 
so as your test exercises VM.class_hierarchy it
should go in 
.../dcmd/vm/ as opposed to just .../dcmd/. 

Thanks, 
Mikael 

  [0] 
http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/tip/test/serviceability/dcmd

  
  Ok. I'll pull the latest hs-rt and update the test.
  Thanks for pointing that out. 
  
  Chris 


  
  

  

  
  


  

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

Looks good! 

Thanks for providing incremental webrevs - very helpful!


Thanks,
/Staffan



> On 12 feb 2015, at 09:31, Chris Plummer  wrote:
> 
> I suppose it would be nice if I included links to the webrevs:
> 
> http://cr.openjdk.java.net/~cjplummer/8054888/webrev.02-03/ 
> 
> http://cr.openjdk.java.net/~cjplummer/8054888/webrev.03/ 
> 
> 
> First one is just the latest changes described in the previous email. 2nd one 
> includes all changes.
> 
> thanks,
> 
> Chris
> 
> On 2/12/15 12:28 AM, Chris Plummer wrote:
>> Ok, hopefully the last webrev. :)
>> 
>> -JPRT found a compiler error on Solaris. classfile/systemDictionary.hpp 
>> needed to be included due to the reference to 
>> SystemDictionary::Object_klass(). I assume this turned up on Solaris because 
>> it doesn't use precompiled headers.
>> 
>> -I noticed I had inadvertently removed a ResourceMark in 
>> KlassInfoHisto::print_class_stats(). I think maybe I had done a cut-n-paste 
>> rather than copy-n-paste. This was in webrev.01 but was pretty inconspicuous 
>> in the webrev so went noticed.
>> 
>> -I changed another WARNING to ERROR as Staffan requested.
>> 
>> -I updated to the latest JDK9 sources and made the needed changes to the 
>> DCMD test as Mikael requested.
>> 
>> thanks,
>> 
>> Chris
>> 
>> On 2/11/15 9:38 AM, Chris Plummer wrote:
>>> On 2/11/15 2:12 AM, Mikael Auno wrote: 
 On 2015-02-11 04:13, Chris Plummer wrote: 
>> In general I think this looks very good. Simple and well-commented 
>> code to follow. I am missing a test, though. Please look at the 
>> hotspot/test/serviceability/dcmd set of tests. 
> Added. 
 Your test is based on DcmdUtil.java which was removed last week (see 
 [0]). If you pull new changes from hs-rt/hotspot, you'll see plenty of 
 tests using the new DCMD utility classes in testlibrary. Also, the new 
 tests are divided in different subdirectories depending on the commands, 
 so as your test exercises VM.class_hierarchy it should go in 
 .../dcmd/vm/ as opposed to just .../dcmd/. 
 
 Thanks, 
 Mikael 
 
   [0] 
 http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/tip/test/serviceability/dcmd
  
 
  
>>> Ok. I'll pull the latest hs-rt and update the test. Thanks for pointing 
>>> that out. 
>>> 
>>> Chris 
>> 
> 

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
I suppose it would be nice if I
  included links to the webrevs:
  
  http://cr.openjdk.java.net/~cjplummer/8054888/webrev.02-03/
  http://cr.openjdk.java.net/~cjplummer/8054888/webrev.03/
  
  First one is just the latest changes described in the previous
  email. 2nd one includes all changes.
  
  thanks,
  
  Chris
  
  On 2/12/15 12:28 AM, Chris Plummer wrote:


  
  Ok, hopefully the last webrev. :)

-JPRT found a compiler error on Solaris.

classfile/systemDictionary.hpp needed to be included due to the
reference to

SystemDictionary::Object_klass(). I assume this turned up on
Solaris because it doesn't use precompiled headers.

-I noticed I had inadvertently removed a ResourceMark in
KlassInfoHisto::print_class_stats(). I think maybe I had done a
cut-n-paste rather than copy-n-paste. This was in webrev.01 but
was pretty inconspicuous in the webrev so went noticed.

-I changed another WARNING to ERROR as Staffan requested.

-I updated to the latest JDK9 sources and made the needed
changes to the DCMD test as Mikael requested.

thanks,

Chris

On 2/11/15 9:38 AM, Chris Plummer wrote:
  
  On
2/11/15 2:12 AM, Mikael Auno wrote: 
On 2015-02-11 04:13, Chris Plummer
  wrote: 
  
In general I think this looks very
  good. Simple and well-commented 
  code to follow. I am missing a test, though. Please look
  at the 
  hotspot/test/serviceability/dcmd set of tests. 

Added. 
  
  Your test is based on DcmdUtil.java which was removed last
  week (see 
  [0]). If you pull new changes from hs-rt/hotspot, you'll see
  plenty of 
  tests using the new DCMD utility classes in testlibrary. Also,
  the new 
  tests are divided in different subdirectories depending on the
  commands, 
  so as your test exercises VM.class_hierarchy it should go in 
  .../dcmd/vm/ as opposed to just .../dcmd/. 
  
  Thanks, 
  Mikael 
  
    [0] 
  http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/tip/test/serviceability/dcmd
  

Ok. I'll pull the latest hs-rt and update the test. Thanks for
pointing that out. 

Chris 
  
  


  

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
Ok, hopefully the last webrev. :)
  
  -JPRT found a compiler error on Solaris.
  
  classfile/systemDictionary.hpp needed to be included due to the
  reference to
  
  SystemDictionary::Object_klass(). I assume this turned up on
  Solaris because it doesn't use precompiled headers.
  
  -I noticed I had inadvertently removed a ResourceMark in
  KlassInfoHisto::print_class_stats(). I think maybe I had done a
  cut-n-paste rather than copy-n-paste. This was in webrev.01 but
  was pretty inconspicuous in the webrev so went noticed.
  
  -I changed another WARNING to ERROR as Staffan requested.
  
  -I updated to the latest JDK9 sources and made the needed changes
  to the DCMD test as Mikael requested.
  
  thanks,
  
  Chris
  
  On 2/11/15 9:38 AM, Chris Plummer wrote:

On
  2/11/15 2:12 AM, Mikael Auno wrote:
  
  On 2015-02-11 04:13, Chris Plummer wrote:


  In general I think this looks very
good. Simple and well-commented

code to follow. I am missing a test, though. Please look at
the

hotspot/test/serviceability/dcmd set of tests.

  
  Added.
  

Your test is based on DcmdUtil.java which was removed last week
(see

[0]). If you pull new changes from hs-rt/hotspot, you'll see
plenty of

tests using the new DCMD utility classes in testlibrary. Also,
the new

tests are divided in different subdirectories depending on the
commands,

so as your test exercises VM.class_hierarchy it should go in

.../dcmd/vm/ as opposed to just .../dcmd/.


Thanks,

Mikael


  [0]

http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/tip/test/serviceability/dcmd

  
  Ok. I'll pull the latest hs-rt and update the test. Thanks for
  pointing that out.
  
  
  Chris
  


  

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
Yes, so I reverted back to the CLD*.
  Seems to be the only thing that is unique per ClassLoader instance
  and does not move.
  
  Chris
  
  On 2/11/15 2:11 PM, Karen Kinnear wrote:

Chris,
  
  
  Just saw this. I was thinking the instance in the heap since
you might have multiple instances of a single subclass.
  Good point that it might change and so the correlation would
be gone.
  
  
  thanks,
  Karen
  

  On Feb 6, 2015, at 5:09 PM, Chris Plummer wrote:
  
  


  Hi Karen,

Can you clarify what you mean by "address of the
j.l.ClassLoader class". Is that the Klass* for the
ClassLoader subclass, or the actual address of the
ClassLoader instance in the heap, which can change.

Chris

On 2/6/15 1:21 PM, Karen Kinnear wrote:
  
  Chris,


So I was curious - I was thinking you would
  printing the hex address of the j.l.ClassLoader class
  rather than the cld so that if folks were to look
at a heap dump later it might be meaningful to
  them. Is it unlikely that they would ever want to
  correlate those?



  
On Feb 6, 2015, at 3:15 PM, Chris Plummer
  wrote:


  
  
I was also thinking
  it might be a good idea to indicate what the
  hex value is, although still have figured out
  the best way of doing this. Maybe just a
  simple comment before the output. Keep in mind
  that eventually other DCMDS will also include
  the cld to help uniquiely identify classes
  across dcmd output. Also keep in mind your
  earlier suggestion:
  
  
  
java.lang.Object/0x12345600
|--java.io.Serializable/0x12345601
|--java.util.RandomAccess/0x12345602
|--java.lang.Iterable/0x12345603
|
|--java.util.Collection/0x12345604
|
| |--java.util.List/0x12345605
|--java.util.AbstractCollection/0x12345606
|
|  implements java.util.Collection/0x12345604
|
|  implements java.lang.Iterable/0x12345603
|
|--java.util.AbstractList/0x12345607
|
|    implements java.util.List/0x12345605
|
| |--java.util.Arrays$ArrayList/0x12345608
|
| |    implements java.io.Serializable/0x12345601
|
| |    implements java.util.RandomAccess/0x12345602
  
  

  
With additions to
  GC.class_stats:
  
  
  
  
Index

Super ClassLoader        ClassName
 
  1    -1 0x0007c0034c48 java.lang.Object/0x12345600

      2     1
  0x0007c0034c48 java.util.List/0x12345605


      3  
   31 0x0007c0034c48 java.util.AbstractList/0x12345607



  
  
And GC.class_histogram:
  
  
  
  
 num
    #instances         #bytes  class
name
-

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Karen Kinnear]

So I am in agreement on the CLD* - sorry for the earlier suggestion.

thanks,
Karen

On Feb 11, 2015, at 3:04 AM, Staffan Larsen wrote:

> This version looks good to me!
> 
> Small comments inline.
> 
>> On 11 feb 2015, at 04:13, Chris Plummer  wrote:
>> 
>> Hi Staffan,
>> 
>> Thanks for your feedback. A new incremental webrev can be found at:
>> 
>> http://cr.openjdk.java.net/~cjplummer/8054888/webrev.01-02/
>> 
>> Most changes are discussed inline below. Here are a couple of additional 
>> changes:
>> 
>> - Changed "transitive interface" to "inherited interface" in the output.
>> - Went back to using the CLD* instead of the Klass*, except still use "null" 
>> for the bootstrap ClassLoader.
> 
> Good. 
> 
>> 
>> On 2/10/15 4:19 AM, Staffan Larsen wrote:
>>> Chris,
>>> 
>>> In general I think this looks very good. Simple and well-commented code to 
>>> follow. I am missing a test, though. Please look at the 
>>> hotspot/test/serviceability/dcmd set of tests.
>> Added.
> 
> Thanks!
> 
>>> 
>>> 
>>> A couple of smaller comments:
>>> 
>>> 
>>> Are Unsafe.defineAnonymousClass classes included? Should they be?
>> I didn't really understand what you were talking about at first, but then 
>> when I started looking at ClassLoaderStatsTest.java, I saw the following:
>> 
>> class TestClass {
>>   static {
>>   // force creation of anonymous class (for the lambdaform)
>>   Runnable r = () -> System.out.println("Hello");
>>   r.run();
>>   }
>> }
>> 
>> I added something similar to my test case and found a whole bunch of lines 
>> like the following are suddenly added to the output:
>> 
>> |--java.lang.invoke.LambdaForm$MH/11440528/null
>> 
>> And there is one that seems specifically for the test case:
>> 
>> |--DcmdTestClass$$Lambda$1/4081552/0xa529fbb0
>> 
>> So I think they answer is yes they are added. As for whether or not they 
>> should be, I really don't know. That's probably up to you. GC.class_stats 
>> does include them however.
> 
> I like to include them.
> 
>> 
>> BTW, I do not include array classes. They are intentionally stripped out 
>> since they don't really have relevance in a Class hierarchy analysis. I can 
>> easily add them back in if you want.
> 
> I’m fine with skipping array classes.
> 
>>> I think ClassHierarchyDCmd should include this code as well to restrict 
>>> remote access:
>>>static const JavaPermission permission() {
>>>  JavaPermission p = {"java.lang.management.ManagementPermission",
>>>  "monitor", NULL};
>>>  return p;
>>>}
>> Ok. I was a bit unclear as to when this was really needed.
>>> diagnosticCommand.hpp:278: Missing �if� in the comment
>> Ok.
>>> vm_operations.hpp: Spelling error in �VM_PrintClassHierachry� and 
>>> �PrintClassHierachry�
>> Ok.
>>> vm_operations.hpp:461: Should the complete class be surrounded by "#if 
>>> INCLUDE_SERVICES� ?
>> Yes, but I can't do anything about the reference in the VM_OPS_DO macro, at 
>> least not in a straight forward manner. I think that part is benign, but 
>> will find out when I do a minimal VM build.
>>> heapInspection.hpp:272: The constructor and destructor does not seem to be 
>>> used. Because of that you should also change it to a AllStatic class.
>> Yeah, old code copied from HeapInspection class. I made it AllStatic and 
>> removed the constructor and destructor. However, my lack of C++ experience 
>> is showing here, and I haven't been happy about the existence of this 
>> KlassHierarchy class. It's static with just one public API. It's purpose in 
>> life is just to allow a VM operation to call that public method, but it just 
>> as easily could have been a regular C function call. Likewise the two 
>> private static methods in KlassHierarchy could have been C functions. There 
>> is no encapsulation or subclassing going on here. If you have 
>> recommendations for further improvement I'm open to suggestions. Otherwise 
>> I'll leave it with just the changes mentioned.
> 
> I come from a C background as well, so I don’t have much to add here. I think 
> this looks reasonable.
> 
>>> heapInspection.cpp:339: Shouldn�t this be labeled as an �error�?
>> That would probably be better. I had copied it from line 742. Should I make 
>> that one an ERROR also to be consistent?
> 
> Yes, please.
> 
> Thanks,
> /Staffan
> 
>> 
>> thanks,
>> 
>> Chris
>>> 
>>> Thanks,
>>> /Staffan
>>> 
>>> 
>>> 
>>> 
 On 10 feb 2015, at 03:00, Chris Plummer  wrote:
 
 [Once again the attachment went out but the main body was stripped. Not 
 too sure what's going on, but here it is again. Sorry if you are getting 
 this twice.]
 
 I've attached updated output:
 
 � I now use the Klass* of the ClassLoader instead of the CLD*, and this 
 is documented in the help output.
 � The Klass* of the ClassLoader now immediately follows the class name, 
 and is also included when printing interface names.
 
 The webrevs can be found 

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Karen Kinnear]

Chris,

Just saw this. I was thinking the instance in the heap since you might have 
multiple instances of a single subclass.
Good point that it might change and so the correlation would be gone.

thanks,
Karen

On Feb 6, 2015, at 5:09 PM, Chris Plummer wrote:

> Hi Karen,
> 
> Can you clarify what you mean by "address of the j.l.ClassLoader class". Is 
> that the Klass* for the ClassLoader subclass, or the actual address of the 
> ClassLoader instance in the heap, which can change.
> 
> Chris
> 
> On 2/6/15 1:21 PM, Karen Kinnear wrote:
>> Chris,
>> 
>> So I was curious - I was thinking you would printing the hex address of the 
>> j.l.ClassLoader class rather than the cld so that if folks were to look
>> at a heap dump later it might be meaningful to them. Is it unlikely that 
>> they would ever want to correlate those?
>> 
>> 
>> On Feb 6, 2015, at 3:15 PM, Chris Plummer wrote:
>> 
>>> I was also thinking it might be a good idea to indicate what the hex value 
>>> is, although still have figured out the best way of doing 
>>> this. Maybe just a simple comment before the output. Keep in mind that 
>>> eventually other DCMDS will also include the cld to help uniquiely identify 
>>> classes across dcmd output. Also keep in mind your earlier suggestion:
>>> 
>>> java.lang.Object/0x12345600
>>> |--java.io.Serializable/0x12345601
>>> |--java.util.RandomAccess/0x12345602
>>> |--java.lang.Iterable/0x12345603
>>> | |--java.util.Collection/0x12345604
>>> | | |--java.util.List/0x12345605
>>> |--java.util.AbstractCollection/0x12345606
>>> | |  implements java.util.Collection/0x12345604
>>> | |  implements java.lang.Iterable/0x12345603
>>> | |--java.util.AbstractList/0x12345607
>>> | |implements java.util.List/0x12345605
>>> | | |--java.util.Arrays$ArrayList/0x12345608
>>> | | |implements java.io.Serializable/0x12345601
>>> | | |implements java.util.RandomAccess/0x12345602
>>> 
>>> With additions to GC.class_stats:
>>> 
>>> Index Super ClassLoaderClassName
>>> 1-1 0x0007c0034c48 java.lang.Object/0x12345600
>>> 2 1 0x0007c0034c48 java.util.List/0x12345605
>>> 331 0x0007c0034c48 java.util.AbstractList/0x12345607
>>> 
>>> And GC.class_histogram:
>>> 
>>>  num #instances #bytes  class name
>>> --
>>>1:   945 117736  java.lang.Object/0x12345600
>>>2:   442  50352  java.util.List/0x12345605
>>>3:   499  25288  java.util.AbstractList/0x12345607
>>> 
>>> I think in your case you assumed we would create a unique identifier for 
>>> each class, but then we settled on just classname + ClassLoader identifier 
>>> of some sort, and the CLD* works for that. So replace your hex class ID in 
>>> the above with the hex CLD*.
>>> 
>>> Also something Karen had said is just now starting to sink in and make 
>>> sense to me:
>>> 
>>> |-sun.misc.Launcher$AppClassLoader/null (0xyyy)
>>> |-myapp/0xyyy
>>> 
>>> I think the idea here is that when printing a ClassLoader subclass, you 
>>> include two CLDs. The first is for the ClassLoader that loaded the class, 
>>> and the second is the CLD for the ClassLoader subclass itself. Thus the 
>>> above indicates that myapp was loaded by 0xyyy, which is the 
>>> sun.misc.Launcher$AppClassLoader, which was loaded by the null ClassLoader. 
>> I apologize - I don't remember what I said. But I am not sure we need that 
>> level of complexity. I think I was asking that one of the dcmds that gives
>> information could print information like 0xabc "a 
>> sun.misc.Launcher$AppClassLoader" or "a WLS.blah.GenericClassLoader" if we 
>> don't want to include
>> that everywhere. Mostly people want to know the name of the class loader. 
>> Since they don't yet have names, they want to know the type of the
>> classloader. So that string "a MyClassLoader" would be more meaningful than 
>> the 0xabc - except that if they
>> have 5 of those they need the uniqueness. So personally I think I was 
>> proposing the 
>>   java.base, 0xA/"a MyClassLoader", iface).
>> 
>> But I would defer to Staffan if he suggests something else.
>> 
>> thanks for your patience,
>> Karen
>>> 
>>> With regard to the '/' format, keep in mind that we also need an indicator 
>>> of whether or not it's an interface, and there's also a request to include 
>>> the module name when that becomes available. At one point you requested the 
>>> following, which is what I was aiming for:
>>> 
>>> java.lang.Object
>>> |--java.io.Serializable (java.base, 0x0007c00375f8, iface)
>>> |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
>>> 
>>> So maybe I can do:
>>> 
>>> java.lang.Object
>>> |--java.io.Serializable/0x0007c00375f8 (java.base, intf)
>>> |--java.util.RandomAccess/0x0007c00375f8 (java.base, intf)
>>> ...
>>> |-sun.misc.Launcher$AppClassLoader/0x0007c00375f8 (java.base, 
>>> CLD=0x00087654320)
>>> |-myapp/0x00087654320
>>> 

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

On 2/11/15 2:12 AM, Mikael Auno wrote:

On 2015-02-11 04:13, Chris Plummer wrote:

In general I think this looks very good. Simple and well-commented
code to follow. I am missing a test, though. Please look at the
hotspot/test/serviceability/dcmd set of tests.

Added.

Your test is based on DcmdUtil.java which was removed last week (see
[0]). If you pull new changes from hs-rt/hotspot, you'll see plenty of
tests using the new DCMD utility classes in testlibrary. Also, the new
tests are divided in different subdirectories depending on the commands,
so as your test exercises VM.class_hierarchy it should go in
.../dcmd/vm/ as opposed to just .../dcmd/.

Thanks,
Mikael

  [0]
http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/tip/test/serviceability/dcmd
Ok. I'll pull the latest hs-rt and update the test. Thanks for pointing 
that out.


Chris

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Mikael Auno]

On 2015-02-11 04:13, Chris Plummer wrote:
>> In general I think this looks very good. Simple and well-commented
>> code to follow. I am missing a test, though. Please look at the
>> hotspot/test/serviceability/dcmd set of tests.
> Added.

Your test is based on DcmdUtil.java which was removed last week (see
[0]). If you pull new changes from hs-rt/hotspot, you'll see plenty of
tests using the new DCMD utility classes in testlibrary. Also, the new
tests are divided in different subdirectories depending on the commands,
so as your test exercises VM.class_hierarchy it should go in
.../dcmd/vm/ as opposed to just .../dcmd/.

Thanks,
Mikael

 [0]
http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/tip/test/serviceability/dcmd

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

This version looks good to me!

Small comments inline.

> On 11 feb 2015, at 04:13, Chris Plummer  wrote:
> 
> Hi Staffan,
> 
> Thanks for your feedback. A new incremental webrev can be found at:
> 
> http://cr.openjdk.java.net/~cjplummer/8054888/webrev.01-02/
> 
> Most changes are discussed inline below. Here are a couple of additional 
> changes:
> 
> - Changed "transitive interface" to "inherited interface" in the output.
> - Went back to using the CLD* instead of the Klass*, except still use "null" 
> for the bootstrap ClassLoader.

Good. 

> 
> On 2/10/15 4:19 AM, Staffan Larsen wrote:
>> Chris,
>> 
>> In general I think this looks very good. Simple and well-commented code to 
>> follow. I am missing a test, though. Please look at the 
>> hotspot/test/serviceability/dcmd set of tests.
> Added.

Thanks!

>> 
>> 
>> A couple of smaller comments:
>> 
>> 
>> Are Unsafe.defineAnonymousClass classes included? Should they be?
> I didn't really understand what you were talking about at first, but then 
> when I started looking at ClassLoaderStatsTest.java, I saw the following:
> 
> class TestClass {
>static {
>// force creation of anonymous class (for the lambdaform)
>Runnable r = () -> System.out.println("Hello");
>r.run();
>}
> }
> 
> I added something similar to my test case and found a whole bunch of lines 
> like the following are suddenly added to the output:
> 
> |--java.lang.invoke.LambdaForm$MH/11440528/null
> 
> And there is one that seems specifically for the test case:
> 
> |--DcmdTestClass$$Lambda$1/4081552/0xa529fbb0
> 
> So I think they answer is yes they are added. As for whether or not they 
> should be, I really don't know. That's probably up to you. GC.class_stats 
> does include them however.

I like to include them.

> 
> BTW, I do not include array classes. They are intentionally stripped out 
> since they don't really have relevance in a Class hierarchy analysis. I can 
> easily add them back in if you want.

I’m fine with skipping array classes.

>> I think ClassHierarchyDCmd should include this code as well to restrict 
>> remote access:
>> static const JavaPermission permission() {
>>   JavaPermission p = {"java.lang.management.ManagementPermission",
>>   "monitor", NULL};
>>   return p;
>> }
> Ok. I was a bit unclear as to when this was really needed.
>> diagnosticCommand.hpp:278: Missing �if� in the comment
> Ok.
>> vm_operations.hpp: Spelling error in �VM_PrintClassHierachry� and 
>> �PrintClassHierachry�
> Ok.
>> vm_operations.hpp:461: Should the complete class be surrounded by "#if 
>> INCLUDE_SERVICES� ?
> Yes, but I can't do anything about the reference in the VM_OPS_DO macro, at 
> least not in a straight forward manner. I think that part is benign, but will 
> find out when I do a minimal VM build.
>> heapInspection.hpp:272: The constructor and destructor does not seem to be 
>> used. Because of that you should also change it to a AllStatic class.
> Yeah, old code copied from HeapInspection class. I made it AllStatic and 
> removed the constructor and destructor. However, my lack of C++ experience is 
> showing here, and I haven't been happy about the existence of this 
> KlassHierarchy class. It's static with just one public API. It's purpose in 
> life is just to allow a VM operation to call that public method, but it just 
> as easily could have been a regular C function call. Likewise the two private 
> static methods in KlassHierarchy could have been C functions. There is no 
> encapsulation or subclassing going on here. If you have recommendations for 
> further improvement I'm open to suggestions. Otherwise I'll leave it with 
> just the changes mentioned.

I come from a C background as well, so I don’t have much to add here. I think 
this looks reasonable.

>> heapInspection.cpp:339: Shouldn�t this be labeled as an �error�?
> That would probably be better. I had copied it from line 742. Should I make 
> that one an ERROR also to be consistent?

Yes, please.

Thanks,
/Staffan

> 
> thanks,
> 
> Chris
>> 
>> Thanks,
>> /Staffan
>> 
>> 
>> 
>>  
>>> On 10 feb 2015, at 03:00, Chris Plummer  wrote:
>>> 
>>> [Once again the attachment went out but the main body was stripped. Not too 
>>> sure what's going on, but here it is again. Sorry if you are getting this 
>>> twice.]
>>> 
>>> I've attached updated output:
>>> 
>>> � I now use the Klass* of the ClassLoader instead of the CLD*, and this 
>>> is documented in the help output.
>>> � The Klass* of the ClassLoader now immediately follows the class name, 
>>> and is also included when printing interface names.
>>> 
>>> The webrevs can be found at:
>>> 
>>> http://cr.openjdk.java.net/~cjplummer/8054888/webrev.01/
>>> http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00-01/
>>> 
>>> The first is the full webrev. The 2nd is what's changed since the last 
>>> webrev that was reviewed. Changes since then include:
>>> 
>>> � Suppo

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

Hi Staffan,

Thanks for your feedback. A new incremental webrev can be found at:

http://cr.openjdk.java.net/~cjplummer/8054888/webrev.01-02/

Most changes are discussed inline below. Here are a couple of additional 
changes:


 - Changed "transitive interface" to "inherited interface" in the output.
 - Went back to using the CLD* instead of the Klass*, except still use 
"null" for the bootstrap ClassLoader.


On 2/10/15 4:19 AM, Staffan Larsen wrote:

Chris,

In general I think this looks very good. Simple and well-commented code to 
follow. I am missing a test, though. Please look at the 
hotspot/test/serviceability/dcmd set of tests.

Added.



A couple of smaller comments:


Are Unsafe.defineAnonymousClass classes included? Should they be?
I didn't really understand what you were talking about at first, but 
then when I started looking at ClassLoaderStatsTest.java, I saw the 
following:


class TestClass {
static {
// force creation of anonymous class (for the lambdaform)
Runnable r = () -> System.out.println("Hello");
r.run();
}
}

I added something similar to my test case and found a whole bunch of 
lines like the following are suddenly added to the output:


|--java.lang.invoke.LambdaForm$MH/11440528/null

And there is one that seems specifically for the test case:

|--DcmdTestClass$$Lambda$1/4081552/0xa529fbb0

So I think they answer is yes they are added. As for whether or not they 
should be, I really don't know. That's probably up to you. 
GC.class_stats does include them however.


BTW, I do not include array classes. They are intentionally stripped out 
since they don't really have relevance in a Class hierarchy analysis. I 
can easily add them back in if you want.

I think ClassHierarchyDCmd should include this code as well to restrict remote 
access:
 static const JavaPermission permission() {
   JavaPermission p = {"java.lang.management.ManagementPermission",
   "monitor", NULL};
   return p;
 }

Ok. I was a bit unclear as to when this was really needed.

diagnosticCommand.hpp:278: Missing �if� in the comment

Ok.

vm_operations.hpp: Spelling error in �VM_PrintClassHierachry� and 
�PrintClassHierachry�

Ok.

vm_operations.hpp:461: Should the complete class be surrounded by "#if 
INCLUDE_SERVICES� ?
Yes, but I can't do anything about the reference in the VM_OPS_DO macro, 
at least not in a straight forward manner. I think that part is benign, 
but will find out when I do a minimal VM build.

heapInspection.hpp:272: The constructor and destructor does not seem to be 
used. Because of that you should also change it to a AllStatic class.
Yeah, old code copied from HeapInspection class. I made it AllStatic and 
removed the constructor and destructor. However, my lack of C++ 
experience is showing here, and I haven't been happy about the existence 
of this KlassHierarchy class. It's static with just one public API. It's 
purpose in life is just to allow a VM operation to call that public 
method, but it just as easily could have been a regular C function call. 
Likewise the two private static methods in KlassHierarchy could have 
been C functions. There is no encapsulation or subclassing going on 
here. If you have recommendations for further improvement I'm open to 
suggestions. Otherwise I'll leave it with just the changes mentioned.

heapInspection.cpp:339: Shouldn�t this be labeled as an �error�?
That would probably be better. I had copied it from line 742. Should I 
make that one an ERROR also to be consistent?


thanks,

Chris


Thanks,
/Staffan



  

On 10 feb 2015, at 03:00, Chris Plummer  wrote:

[Once again the attachment went out but the main body was stripped. Not too 
sure what's going on, but here it is again. Sorry if you are getting this 
twice.]

I've attached updated output:

� I now use the Klass* of the ClassLoader instead of the CLD*, and this is 
documented in the help output.
� The Klass* of the ClassLoader now immediately follows the class name, and 
is also included when printing interface names.

The webrevs can be found at:

http://cr.openjdk.java.net/~cjplummer/8054888/webrev.01/
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00-01/

The first is the full webrev. The 2nd is what's changed since the last webrev 
that was reviewed. Changes since then include:

� Support for printing the hierarchy of just one class.
� -s option for optionally including subclasses when printing one class.
� -i option for optionally including interfaces implemented by a class.
� Output formatting changes.
� Fixed some comment typos as requested.
� I moved a couple of KlassInfoEntry methods out of the .hpp file and into 
the .cpp file as requested.
� No longer keep track of the stack of superclasses when processing all the 
classes as requested. This also means the super_index field I added is no 
longer needed.
� Moved some code within an already existing " #if INCLUDE_SER

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

Chris,

In general I think this looks very good. Simple and well-commented code to 
follow. I am missing a test, though. Please look at the 
hotspot/test/serviceability/dcmd set of tests.


A couple of smaller comments:


Are Unsafe.defineAnonymousClass classes included? Should they be?

I think ClassHierarchyDCmd should include this code as well to restrict remote 
access:
static const JavaPermission permission() {
  JavaPermission p = {"java.lang.management.ManagementPermission",
  "monitor", NULL};
  return p;
}

diagnosticCommand.hpp:278: Missing “if” in the comment

vm_operations.hpp: Spelling error in “VM_PrintClassHierachry” and 
“PrintClassHierachry”

vm_operations.hpp:461: Should the complete class be surrounded by "#if 
INCLUDE_SERVICES” ?

heapInspection.hpp:272: The constructor and destructor does not seem to be 
used. Because of that you should also change it to a AllStatic class.

heapInspection.cpp:339: Shouldn’t this be labeled as an “error”?


Thanks,
/Staffan



 
> On 10 feb 2015, at 03:00, Chris Plummer  wrote:
> 
> [Once again the attachment went out but the main body was stripped. Not too 
> sure what's going on, but here it is again. Sorry if you are getting this 
> twice.]
> 
> I've attached updated output:
> 
> • I now use the Klass* of the ClassLoader instead of the CLD*, and this is 
> documented in the help output.
> • The Klass* of the ClassLoader now immediately follows the class name, and 
> is also included when printing interface names.
> 
> The webrevs can be found at:
> 
> http://cr.openjdk.java.net/~cjplummer/8054888/webrev.01/
> http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00-01/
> 
> The first is the full webrev. The 2nd is what's changed since the last webrev 
> that was reviewed. Changes since then include:
> 
> • Support for printing the hierarchy of just one class.
> • -s option for optionally including subclasses when printing one class.
> • -i option for optionally including interfaces implemented by a class.
> • Output formatting changes.
> • Fixed some comment typos as requested.
> • I moved a couple of KlassInfoEntry methods out of the .hpp file and into 
> the .cpp file as requested.
> • No longer keep track of the stack of superclasses when processing all the 
> classes as requested. This also means the super_index field I added is no 
> longer needed.
> • Moved some code within an already existing " #if INCLUDE_SERVICES" block as 
> requested.
> 
> thanks,
> 
> Chris
> 

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

[Once again the attachment went out but the main body was stripped. Not 
too sure what's going on, but here it is again. Sorry if you are getting 
this twice.]


I've attached updated output:

• I now use the Klass* of the ClassLoader instead of the CLD*, and this 
is documented in the help output.
• The Klass* of the ClassLoader now immediately follows the class name, 
and is also included when printing interface names.


The webrevs can be found at:

http://cr.openjdk.java.net/~cjplummer/8054888/webrev.01/
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00-01/

The first is the full webrev. The 2nd is what's changed since the last 
webrev that was reviewed. Changes since then include:


• Support for printing the hierarchy of just one class.
• -s option for optionally including subclasses when printing one class.
• -i option for optionally including interfaces implemented by a class.
• Output formatting changes.
• Fixed some comment typos as requested.
• I moved a couple of KlassInfoEntry methods out of the .hpp file and 
into the .cpp file as requested.
• No longer keep track of the stack of superclasses when processing all 
the classes as requested. This also means the super_index field I added 
is no longer needed.
• Moved some code within an already existing " #if INCLUDE_SERVICES" 
block as requested.


thanks,

Chris

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

Arrg. Just noticed these emails after I sent my webrev. They got snagged 
by my openjdk filter for later viewing. I had been expecting all replies 
to CC me and be in my inbox. serviceability-dev@openjdk.java.net also 
got removed, so they've been re-added.


Anyway, good point on both j.l.ClassLoader address suffering from 
objects moving (which is why I went with Klass*), and good point that 
more than one ClassLoader instance can have the same Klass*. I will 
change back to using the CLD*, but still use null for the bootstrap 
classloader since that makes it easier to read the output.


Please still review the webrev I just sent out. The change to CLD* will 
be pretty simple and not impact the existing logic. I can send out an 
incremental webrev to cover changes for it and any addition work that 
may still be needed.


thanks,

Chris

On 2/9/15 11:33 AM, Staffan Larsen wrote:

Very good point! Let’s go with ClassLoaderData.

/Staffan


On 9 feb 2015, at 18:35, Ioi Lam  wrote:

Hmm, you may have two instances of the same j.l.ClassLoader type, and their 
Klass will have the same value. This means you can't distinguish between the 
ClassLoader instances.

So, maybe we should change all the jcmd code to print a unique identifier for 
each ClassLoader instance. ClassLoaderData seems a good choice :-)

- Ioi


On 2/9/15, 1:40 AM, Staffan Larsen wrote:

Using the address of the Klass of the j.l.ClassLoader is good. It is also 
printed by VM.classloader_stats so allows for correlation there too. (Note that 
printing address of the j.l.ClassLoader itself is not a good idea since that 
can change during the lifetime of the JVM).

/Staffan


On 9 feb 2015, at 06:47, Ioi Lam  wrote:

I agree that we should use the address of the j.l.ClassLoader. This way, we can 
match the output with that of GC.class_stats, which prints out the class loader 
as:

class loader 0x7fc09c11d3d0a 'sun/misc/Launcher$AppClassLoader'

Thanks
- Ioi

On 2/6/15, 1:21 PM, Karen Kinnear wrote:

Chris,

So I was curious - I was thinking you would printing the hex address of the 
j.l.ClassLoader class rather than the cld so that if folks were to look
at a heap dump later it might be meaningful to them. Is it unlikely that they 
would ever want to correlate those?


On Feb 6, 2015, at 3:15 PM, Chris Plummer wrote:


I was also thinking it might be a good idea to indicate what the hex value is, 
although still have figured out the best way of doing this. Maybe just a simple 
comment before the output. Keep in mind that eventually other DCMDS will also 
include the cld to help uniquiely identify classes across dcmd output. Also 
keep in mind your earlier suggestion:

java.lang.Object/0x12345600
|--java.io.Serializable/0x12345601
|--java.util.RandomAccess/0x12345602
|--java.lang.Iterable/0x12345603
| |--java.util.Collection/0x12345604
| | |--java.util.List/0x12345605
|--java.util.AbstractCollection/0x12345606
| |  implements java.util.Collection/0x12345604
| |  implements java.lang.Iterable/0x12345603
| |--java.util.AbstractList/0x12345607
| |implements java.util.List/0x12345605
| | |--java.util.Arrays$ArrayList/0x12345608
| | |implements java.io.Serializable/0x12345601
| | |implements java.util.RandomAccess/0x12345602

With additions to GC.class_stats:

Index Super ClassLoaderClassName
 1-1 0x0007c0034c48 java.lang.Object/0x12345600
 2 1 0x0007c0034c48 java.util.List/0x12345605
 331 0x0007c0034c48 java.util.AbstractList/0x12345607

And GC.class_histogram:

  num #instances #bytes  class name
--
1:   945 117736  java.lang.Object/0x12345600
2:   442  50352  java.util.List/0x12345605
3:   499  25288  java.util.AbstractList/0x12345607

I think in your case you assumed we would create a unique identifier for each 
class, but then we settled on just classname + ClassLoader identifier of some 
sort, and the CLD* works for that. So replace your hex class ID in the above 
with the hex CLD*.

Also something Karen had said is just now starting to sink in and make sense to 
me:

|-sun.misc.Launcher$AppClassLoader/null (0xyyy)
|-myapp/0xyyy

I think the idea here is that when printing a ClassLoader subclass, you include 
two CLDs. The first is for the ClassLoader that loaded the class, and the 
second is the CLD for the ClassLoader subclass itself. Thus the above indicates 
that myapp was loaded by 0xyyy, which is the sun.misc.Launcher$AppClassLoader, 
which was loaded by the null ClassLoader.

I apologize - I don't remember what I said. But I am not sure we need that 
level of complexity. I think I was asking that one of the dcmds that gives
information could print information like 0xabc "a sun.misc.Launcher$AppClassLoader" or 
"a WLS.blah.GenericClassLoader" if we don't want to include
that everywhere. Mostly people want to know the name of the class loader. Since 
they don't yet h

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
I've attached updated output:

  I now use the Klass* of the ClassLoader instead of the CLD*,
and this is documented in the help output.
  
  The Klass* of the ClassLoader now immediately follows the
class name, and is also included when printing interface names.

The webrevs can be found at:

http://cr.openjdk.java.net/~cjplummer/8054888/webrev.01/
  http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00-01/

The first is the full webrev. The 2nd is what's changed since the
last webrev that was reviewed. Changes since then include:

  Support for printing the hierarchy of just one class.
  -s option for optionally including subclasses when printing
one class.
  -i option for optionally including interfaces implemented by a
class.
  Output formatting changes.
  Fixed some comment typos as requested.
  
  I moved a couple of KlassInfoEntry methods out of the .hpp
file and into the .cpp file as requested.
  No longer keep track of the stack of superclasses when
processing all the classes as requested. This also means the
super_index field I added is no longer needed.
  Moved some code within an already existing "

#if INCLUDE_SERVICES" block as requested.
  

thanks,
Chris

On 2/5/15 6:49 PM, Chris Plummer wrote:
> Hey Folks,
>
> Sorry about the delay in getting the next webrev for this out.
I was sidetracked by a few other things, including being out sick of
almost a week, and there were also quite a few changes to make.
>
> I'm ready for review with an updated webrev, but thought first
I'd have you look at and comment on the output. Please see the
attached file. It now supports:
>
> printing the hierarchy for a single class
> optionally including all subclasses of the specified class
(-s)
> for each class, also list all of its interfaces (-i)
>
> The hex value in the output is the address of the
ClassLoaderData for the ClassLoader of the class. I did not include
the address of the Klass, but could if you think it would be useful.
Changing the format of what comes after the classname is easy. Just
let me know what you think is best.
>
> I have not updated any other dcmds to be consistent with how
classes are uniquely identified. A separate bug should be filed for
that. Actually I thought one was, but I looked through this thread's
history and could not find mention of it. I also have not
implemented the reverse hierarchy dcmd. JDK-8068830 has already been
filed for that, but there are no plans to work on it in the near
term.
>
> thanks,
>
> Chris
>
> On 1/7/15 3:29 PM, Chris Plummer wrote:
>> Hi,
>>
>> Please review the following changes for the addition of the
VM.class_hierarchy DCMD. Please read the bug first for some
background information.
>>
>> Webrev:
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8054888
>>
>> I expect there will be further restructuring or additional
feature work. More discussion on that below. I'm not sure if that
additional work will be done later with a separately bug filed or
with this initial commit. That's one thing I want to work out with
this review.
>>
>> Currently the bulk of the DCMD is implemented in
heapInspection.cpp. The  main purpose of this file is to implement
the GC.class_stats and GC.class_histogram DCMDs. Both of them
require walking the java heap to count live objects of each type,
thus the name "heapInspection.cpp". This new VM.class_hierarchy DCMD
does not require walking the heap, but is implemented in this file
because it leverages the existing KlassInfoTable and related classes
(KlassInfoEntry, KlassInfoBucket, and KlassClosure).
>>
>> KlassInfoTable makes it easy to build a database of all
loaded classes, save additional info gathered for each class,
iterate over them quickly, and also do quick lookups. This exactly
what I needed for this DCMD, thus the reuse. There is some downside
to this. For starters, heapInspection.cpp is not the proper place
for a DCMD that has nothing to do with heap inspection. Also,
KlassInfoEntry is being overloaded now to support 3 different DCMDs,
as is KlassInfoTable. As a result each has a few fields and methods
that are not used for all 3 DCMDs. Some subclassing might be in
order here, but I'm not sure if it's worth it. Opinions welcomed. If
I am going to refactor, I would prefer that be done as a next step
so I'm not disturbing the existing DCMDs with this first
implementation.
>>
>> I added some comments to code only used for GC.class_stats
and GC.class_histogram. I

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

> On 6 feb 2015, at 22:21, Karen Kinnear  wrote:
> 
> Chris,
> 
> So I was curious - I was thinking you would printing the hex address of the 
> j.l.ClassLoader class rather than the cld so that if folks were to look
> at a heap dump later it might be meaningful to them. Is it unlikely that they 
> would ever want to correlate those?
> 
> 
> On Feb 6, 2015, at 3:15 PM, Chris Plummer wrote:
> 
>> I was also thinking it might be a good idea to indicate what the hex value 
>> is, although still have figured out the best way of doing this. Maybe just a 
>> simple comment before the output. Keep in mind that eventually other DCMDS 
>> will also include the cld to help uniquiely identify classes across dcmd 
>> output. Also keep in mind your earlier suggestion:
>> 
>> java.lang.Object/0x12345600
>> |--java.io.Serializable/0x12345601
>> |--java.util.RandomAccess/0x12345602
>> |--java.lang.Iterable/0x12345603
>> | |--java.util.Collection/0x12345604
>> | | |--java.util.List/0x12345605
>> |--java.util.AbstractCollection/0x12345606
>> | |  implements java.util.Collection/0x12345604
>> | |  implements java.lang.Iterable/0x12345603
>> | |--java.util.AbstractList/0x12345607
>> | |implements java.util.List/0x12345605
>> | | |--java.util.Arrays$ArrayList/0x12345608
>> | | |implements java.io.Serializable/0x12345601
>> | | |implements java.util.RandomAccess/0x12345602
>> 
>> With additions to GC.class_stats:
>> 
>> Index Super ClassLoaderClassName
>> 1-1 0x0007c0034c48 java.lang.Object/0x12345600
>> 2 1 0x0007c0034c48 java.util.List/0x12345605
>> 331 0x0007c0034c48 java.util.AbstractList/0x12345607
>> 
>> And GC.class_histogram:
>> 
>>  num #instances #bytes  class name
>> --
>>1:   945 117736  java.lang.Object/0x12345600
>>2:   442  50352  java.util.List/0x12345605
>>3:   499  25288  java.util.AbstractList/0x12345607
>> 
>> I think in your case you assumed we would create a unique identifier for 
>> each class, but then we settled on just classname + ClassLoader identifier 
>> of some sort, and the CLD* works for that. So replace your hex class ID in 
>> the above with the hex CLD*.
>> 
>> Also something Karen had said is just now starting to sink in and make sense 
>> to me:
>> 
>> |-sun.misc.Launcher$AppClassLoader/null (0xyyy)
>> |-myapp/0xyyy
>> 
>> I think the idea here is that when printing a ClassLoader subclass, you 
>> include two CLDs. The first is for the ClassLoader that loaded the class, 
>> and the second is the CLD for the ClassLoader subclass itself. Thus the 
>> above indicates that myapp was loaded by 0xyyy, which is the 
>> sun.misc.Launcher$AppClassLoader, which was loaded by the null ClassLoader. 
> I apologize - I don't remember what I said. But I am not sure we need that 
> level of complexity. I think I was asking that one of the dcmds that gives
> information could print information like 0xabc "a 
> sun.misc.Launcher$AppClassLoader" or "a WLS.blah.GenericClassLoader" if we 
> don't want to include
> that everywhere. Mostly people want to know the name of the class loader. 
> Since they don't yet have names, they want to know the type of the
> classloader. So that string "a MyClassLoader" would be more meaningful than 
> the 0xabc - except that if they
> have 5 of those they need the uniqueness. So personally I think I was 
> proposing the 
>   java.base, 0xA/"a MyClassLoader", iface).
> 
> But I would defer to Staffan if he suggests something else.

The mapping from “Klass of the j.l.ClassLoader” to “type of the ClassLoader” is 
available in the VM.classloader_stats command:

ClassLoader Parent  CLD*   Classes   ChunkSz   
BlockSz  Type
0x0007c0038d10  0x0007c0036240  0x7faf7843d8f0   1  6144
  1496  sun.misc.Launcher$AppClassLoader
0x0007c0036240  0x  0x   0 0
 0  sun.misc.Launcher$ExtClassLoader
0x  0x  0x7faf7852ffd0 432   4587520   
2851696  
Total = 3  433   4593664   
2853192
ChunkSz: Total size of all allocated metaspace chunks
BlockSz: Total size of all allocated metaspace blocks (each chunk has several 
blocks)

I think that having the mapping in one place is enough.

/Staffan

> 
> thanks for your patience,
> Karen
>> 
>> With regard to the '/' format, keep in mind that we also need an indicator 
>> of whether or not it's an interface, and there's also a request to include 
>> the module name when that becomes available. At one point you requested the 
>> following, which is what I was aiming for:
>> 
>> java.lang.Object
>> |--java.io.Serializable (java.base, 0x0007c00375f8, iface)
>> |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
>> 
>> So maybe I can do:
>> 
>> java.lang.Object
>> |--java.io.Se

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

> On 6 feb 2015, at 21:15, Chris Plummer  wrote:
> 
> I was also thinking it might be a good idea to indicate what the hex value 
> is, although still have figured out the best way of doing this. Maybe just a 
> simple comment before the output. Keep in mind that eventually other DCMDS 
> will also include the cld to help uniquiely identify classes across dcmd 
> output. Also keep in mind your earlier suggestion:
> 
> java.lang.Object/0x12345600
> |--java.io.Serializable/0x12345601
> |--java.util.RandomAccess/0x12345602
> |--java.lang.Iterable/0x12345603
> | |--java.util.Collection/0x12345604
> | | |--java.util.List/0x12345605
> |--java.util.AbstractCollection/0x12345606
> | |  implements java.util.Collection/0x12345604
> | |  implements java.lang.Iterable/0x12345603
> | |--java.util.AbstractList/0x12345607
> | |implements java.util.List/0x12345605
> | | |--java.util.Arrays$ArrayList/0x12345608
> | | |implements java.io.Serializable/0x12345601
> | | |implements java.util.RandomAccess/0x12345602
> 
> With additions to GC.class_stats:
> 
> Index Super ClassLoaderClassName
> 1-1 0x0007c0034c48 java.lang.Object/0x12345600
> 2 1 0x0007c0034c48 java.util.List/0x12345605
> 331 0x0007c0034c48 java.util.AbstractList/0x12345607
> 
> And GC.class_histogram:
> 
>  num #instances #bytes  class name
> --
>1:   945 117736  java.lang.Object/0x12345600
>2:   442  50352  java.util.List/0x12345605
>3:   499  25288  java.util.AbstractList/0x12345607
> 
> I think in your case you assumed we would create a unique identifier for each 
> class, but then we settled on just classname + ClassLoader identifier of some 
> sort, and the CLD* works for that. So replace your hex class ID in the above 
> with the hex CLD*.
> 
> Also something Karen had said is just now starting to sink in and make sense 
> to me:
> 
> |-sun.misc.Launcher$AppClassLoader/null (0xyyy)
> |-myapp/0xyyy
> 
> I think the idea here is that when printing a ClassLoader subclass, you 
> include two CLDs. The first is for the ClassLoader that loaded the class, and 
> the second is the CLD for the ClassLoader subclass itself. Thus the above 
> indicates that myapp was loaded by 0xyyy, which is the 
> sun.misc.Launcher$AppClassLoader, which was loaded by the null ClassLoader. 
> 
> With regard to the '/' format, keep in mind that we also need an indicator of 
> whether or not it's an interface, and there's also a request to include the 
> module name when that becomes available. At one point you requested the 
> following, which is what I was aiming for:
> 
> java.lang.Object
> |--java.io.Serializable (java.base, 0x0007c00375f8, iface)
> |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
> 
> So maybe I can do:
> 
> java.lang.Object
> |--java.io.Serializable/0x0007c00375f8 (java.base, intf)
> |--java.util.RandomAccess/0x0007c00375f8 (java.base, intf)

This looks good to me.

> ...
> |-sun.misc.Launcher$AppClassLoader/0x0007c00375f8 (java.base, 
> CLD=0x00087654320)
> |-myapp/0x00087654320
> 
> So now the classname and its classloader id (which is the CLD*)  are grouped 
> together to make it easy to strip out or to search for in more than one dcmd 
> output. I think this is probably what you were striving when you proposed 
> using '/', and other DCMDs can eventually be changed to do the same. Also, 
> once you know the CLD of the ClassLoader, you can find out the name of the 
> ClassLoader class by looking for CLD=
> 
> I can go with "null" for the null ClassLoader if you want. I used it's 
> ClassLoaderData* to be consistent with the other ClassLoaders. Just let me 
> know which you prefer.

If we change the CLD* to the “Klass of the j.l.ClassLoader”, then it will 
become “null”. If someone wants to find the CLD* for the null ClassLoader, then 
the VM.classloader_stats command will get that.

/Staffan

> 
> thanks,
> 
> Chris
> 
> On 2/6/15 12:52 AM, Staffan Larsen wrote:
>> I think this looks good! Perhaps we should give a hint as to what the hex 
>> value is? I don�t know if it is best to print this as part of a �header� 
>> printed before the rest of the output or if we should include it as part of 
>> each line �(classloaderdata*=0x080609c8)�.
>> 
>> /Staffan
>> 
>>> On 6 feb 2015, at 05:49, Chris Plummer  
>>>  wrote:
>>> 
>>> [Hmm. My previous email somehow included the attachment with the dcmd 
>>> output, but not the body of the message, so here it is again.]
>>> 
>>> Hey Folks,
>>> 
>>> Sorry about the delay in getting the next webrev for this out. I was 
>>> sidetracked by a few other things, including being out sick of almost a 
>>> week, and there were also quite a few changes to make.
>>> 
>>> I'm ready for review with an updated webrev, but thought first I'd have you 
>>> look at and comment on the output. Please 

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
Hi Karen,
  
  Can you clarify what you mean by "address of the j.l.ClassLoader
  class". Is that the Klass* for the ClassLoader subclass, or the
  actual address of the ClassLoader instance in the heap, which can
  change.
  
  Chris
  
  On 2/6/15 1:21 PM, Karen Kinnear wrote:

Chris,
  
  
  So I was curious - I was thinking you would printing the hex
address of the j.l.ClassLoader class rather than the cld so that
if folks were to look
  at a heap dump later it might be meaningful to them. Is it
unlikely that they would ever want to correlate those?
  
  
  

  On Feb 6, 2015, at 3:15 PM, Chris Plummer wrote:
  
  


  I was also thinking it might
be a good idea to indicate what the hex value is,
although still have figured out the best way of doing
this. Maybe just a simple comment before the output.
Keep in mind that eventually other DCMDS will also
include the cld to help uniquiely identify classes
across dcmd output. Also keep in mind your earlier
suggestion:



  java.lang.Object/0x12345600
  |--java.io.Serializable/0x12345601
  |--java.util.RandomAccess/0x12345602
  |--java.lang.Iterable/0x12345603
  |
  |--java.util.Collection/0x12345604
  | |
  |--java.util.List/0x12345605
  |--java.util.AbstractCollection/0x12345606
  | |
   implements java.util.Collection/0x12345604
  | |  implements java.lang.Iterable/0x12345603
  |
  |--java.util.AbstractList/0x12345607
  | |    implements java.util.List/0x12345605
  | |
  |--java.util.Arrays$ArrayList/0x12345608
  | | |    implements java.io.Serializable/0x12345601
  | | |    implements java.util.RandomAccess/0x12345602


  

  With additions to GC.class_stats:




  Index
  Super ClassLoader        ClassName
      1  
   -1 0x0007c0034c48 java.lang.Object/0x12345600
  
    2  
  1 0x0007c0034c48 java.util.List/0x12345605
  
  
    3  
 31 0x0007c0034c48 java.util.AbstractList/0x12345607
  
  
  


  And GC.class_histogram:




   num    
  #instances         #bytes  class name
  --
     1:    
        945         117736  java.lang.Object/0x12345600
     2:    
        442          50352  java.util.List/0x12345605
     3:    
        499          25288  java.util.AbstractList/0x12345607



I think in your case you assumed we would create a
unique identifier for each class, but then we settled on
just classname + ClassLoader identifier of some sort,
and the CLD* works for that. So replace your hex class
ID in the above with the hex CLD*.

Also something Karen had said is just now starting to
sink in and make sense to me:

|-sun.misc.Launcher$AppClassLoader/null (0xyyy)
|-myapp/0xyyy

I think the idea here is that when printing a
ClassLoader subclass, you include two CLDs. The first is
for the ClassLoader that loaded the class, and the
second is the CLD for the ClassLoader subclass itself.
Thus the above indicates that myapp was loaded by 0xyyy,
which is the sun.misc.Launcher$AppClassLoader, which was
loaded by the null ClassLoader. 
  

  
  I apologize - I don't remember what I said. But I am not sure
  we need that level of complexity. I think I was asking that
  one of the dcmds that gives
information could print information like 0xabc "a
  sun.misc.Launcher$AppClassLoader" or "

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Karen Kinnear]

Chris,

So I was curious - I was thinking you would printing the hex address of the 
j.l.ClassLoader class rather than the cld so that if folks were to look
at a heap dump later it might be meaningful to them. Is it unlikely that they 
would ever want to correlate those?


On Feb 6, 2015, at 3:15 PM, Chris Plummer wrote:

> I was also thinking it might be a good idea to indicate what the hex value 
> is, although still have figured out the best way of doing this. Maybe just a 
> simple comment before the output. Keep in mind that eventually other DCMDS 
> will also include the cld to help uniquiely identify classes across dcmd 
> output. Also keep in mind your earlier suggestion:
> 
> java.lang.Object/0x12345600
> |--java.io.Serializable/0x12345601
> |--java.util.RandomAccess/0x12345602
> |--java.lang.Iterable/0x12345603
> | |--java.util.Collection/0x12345604
> | | |--java.util.List/0x12345605
> |--java.util.AbstractCollection/0x12345606
> | |  implements java.util.Collection/0x12345604
> | |  implements java.lang.Iterable/0x12345603
> | |--java.util.AbstractList/0x12345607
> | |implements java.util.List/0x12345605
> | | |--java.util.Arrays$ArrayList/0x12345608
> | | |implements java.io.Serializable/0x12345601
> | | |implements java.util.RandomAccess/0x12345602
> 
> With additions to GC.class_stats:
> 
> Index Super ClassLoaderClassName
> 1-1 0x0007c0034c48 java.lang.Object/0x12345600
> 2 1 0x0007c0034c48 java.util.List/0x12345605
> 331 0x0007c0034c48 java.util.AbstractList/0x12345607
> 
> And GC.class_histogram:
> 
>  num #instances #bytes  class name
> --
>1:   945 117736  java.lang.Object/0x12345600
>2:   442  50352  java.util.List/0x12345605
>3:   499  25288  java.util.AbstractList/0x12345607
> 
> I think in your case you assumed we would create a unique identifier for each 
> class, but then we settled on just classname + ClassLoader identifier of some 
> sort, and the CLD* works for that. So replace your hex class ID in the above 
> with the hex CLD*.
> 
> Also something Karen had said is just now starting to sink in and make sense 
> to me:
> 
> |-sun.misc.Launcher$AppClassLoader/null (0xyyy)
> |-myapp/0xyyy
> 
> I think the idea here is that when printing a ClassLoader subclass, you 
> include two CLDs. The first is for the ClassLoader that loaded the class, and 
> the second is the CLD for the ClassLoader subclass itself. Thus the above 
> indicates that myapp was loaded by 0xyyy, which is the 
> sun.misc.Launcher$AppClassLoader, which was loaded by the null ClassLoader. 
I apologize - I don't remember what I said. But I am not sure we need that 
level of complexity. I think I was asking that one of the dcmds that gives
information could print information like 0xabc "a 
sun.misc.Launcher$AppClassLoader" or "a WLS.blah.GenericClassLoader" if we 
don't want to include
that everywhere. Mostly people want to know the name of the class loader. Since 
they don't yet have names, they want to know the type of the
classloader. So that string "a MyClassLoader" would be more meaningful than the 
0xabc - except that if they
have 5 of those they need the uniqueness. So personally I think I was proposing 
the 
  java.base, 0xA/"a MyClassLoader", iface).

But I would defer to Staffan if he suggests something else.

thanks for your patience,
Karen
> 
> With regard to the '/' format, keep in mind that we also need an indicator of 
> whether or not it's an interface, and there's also a request to include the 
> module name when that becomes available. At one point you requested the 
> following, which is what I was aiming for:
> 
> java.lang.Object
> |--java.io.Serializable (java.base, 0x0007c00375f8, iface)
> |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
> 
> So maybe I can do:
> 
> java.lang.Object
> |--java.io.Serializable/0x0007c00375f8 (java.base, intf)
> |--java.util.RandomAccess/0x0007c00375f8 (java.base, intf)
> ...
> |-sun.misc.Launcher$AppClassLoader/0x0007c00375f8 (java.base, 
> CLD=0x00087654320)
> |-myapp/0x00087654320
> 
> So now the classname and its classloader id (which is the CLD*)  are grouped 
> together to make it easy to strip out or to search for in more than one dcmd 
> output. I think this is probably what you were striving when you proposed 
> using '/', and other DCMDs can eventually be changed to do the same. Also, 
> once you know the CLD of the ClassLoader, you can find out the name of the 
> ClassLoader class by looking for CLD=
> 
> I can go with "null" for the null ClassLoader if you want. I used it's 
> ClassLoaderData* to be consistent with the other ClassLoaders. Just let me 
> know which you prefer.
> 
> thanks,
> 
> Chris
> 
> On 2/6/15 12:52 AM, Staffan Larsen wrote:
>> I think this looks good! Perhaps we should give a hint as to what the hex 
>> value is? I don�t k

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
I was also thinking it might be a good
  idea to indicate what the hex value is, although still have
  figured out the best way of doing this. Maybe just a simple
  comment before the output. Keep in mind that eventually other
  DCMDS will also include the cld to help uniquiely identify classes
  across dcmd output. Also keep in mind your earlier suggestion:
  
  
  
java.lang.Object/0x12345600
|--java.io.Serializable/0x12345601
|--java.util.RandomAccess/0x12345602
|--java.lang.Iterable/0x12345603
|
|--java.util.Collection/0x12345604
| |
|--java.util.List/0x12345605
|--java.util.AbstractCollection/0x12345606
| |  implements
java.util.Collection/0x12345604
| |  implements java.lang.Iterable/0x12345603
|
|--java.util.AbstractList/0x12345607
| |    implements java.util.List/0x12345605
| |
|--java.util.Arrays$ArrayList/0x12345608
| | |    implements java.io.Serializable/0x12345601
| | |    implements java.util.RandomAccess/0x12345602
  
  

  
With additions to GC.class_stats:
  
  
  
  
Index Super
ClassLoader        ClassName
    1    -1
0x0007c0034c48 java.lang.Object/0x12345600

      2     1
  0x0007c0034c48 java.util.List/0x12345605


      3  
   31 0x0007c0034c48 java.util.AbstractList/0x12345607



  
  
And GC.class_histogram:
  
  
  
  
 num     #instances
        #bytes  class name
--
   1:           945
        117736  java.lang.Object/0x12345600
   2:           442
         50352  java.util.List/0x12345605
   3:           499
         25288  java.util.AbstractList/0x12345607
  
  
  
  I think in your case you assumed we would create a unique
  identifier for each class, but then we settled on just classname +
  ClassLoader identifier of some sort, and the CLD* works for that.
  So replace your hex class ID in the above with the hex CLD*.
  
  Also something Karen had said is just now starting to sink in and
  make sense to me:
  
  |-sun.misc.Launcher$AppClassLoader/null (0xyyy)
  |-myapp/0xyyy
  
  I think the idea here is that when printing a ClassLoader
  subclass, you include two CLDs. The first is for the ClassLoader
  that loaded the class, and the second is the CLD for the
  ClassLoader subclass itself. Thus the above indicates that myapp
  was loaded by 0xyyy, which is the
  sun.misc.Launcher$AppClassLoader, which was loaded by the null
  ClassLoader. 
  
  With regard to the '/' format, keep in mind that we also need an
  indicator of whether or not it's an interface, and there's also a
  request to include the module name when that becomes available. At
  one point you requested the following, which is what I was aiming
  for:
  
  java.lang.Object
  |--java.io.Serializable (java.base,
  0x0007c00375f8, iface)
  |--java.util.RandomAccess (java.base,
  0x0007c00375f8, iface)
  
  So maybe I can do:
  
  
java.lang.Object
|--java.io.Serializable/0x0007c00375f8
(java.base, intf)
|--java.util.RandomAccess/0x0007c00375f8
(java.base, intf)
...
  
|-sun.misc.Launcher$AppClassLoader/0x0007c00375f8
(java.base, CLD=0x00087654320)
  |-myapp/0x00087654320

So now the classname and its classloader id (which is the
CLD*)  are grouped together to make it easy to strip out or
to search for in more than one dcmd output. I think this is
probably what you were striving when you proposed using '/',
and other DCMDs can eventually be changed to do the same.
Also, once you know the CLD of the ClassLoader, you can find
out the name of the ClassLoader class by looking for
CLD=

I can go with "null" for the null ClassLoader if you want. I
used it's ClassLoaderData* to be consistent with the other
ClassLoaders. Just let me know which you prefer.

thanks,

Chris
  
  
  On 2/6/15 12:52 AM, Staffan Larsen wrote:


  I think this looks good! Perhaps we should give a hint as to what the hex value is? I don�t know if it is best to print this as part of a �header� printed before the rest of the output or if we should include it as part of each line �(classloaderdata*=0x

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

I think this looks good! Perhaps we should give a hint as to what the hex value 
is? I don’t know if it is best to print this as part of a “header” printed 
before the rest of the output or if we should include it as part of each line 
“(classloaderdata*=0x080609c8)”.

/Staffan

> On 6 feb 2015, at 05:49, Chris Plummer  wrote:
> 
> [Hmm. My previous email somehow included the attachment with the dcmd output, 
> but not the body of the message, so here it is again.]
> 
> Hey Folks,
> 
> Sorry about the delay in getting the next webrev for this out. I was 
> sidetracked by a few other things, including being out sick of almost a week, 
> and there were also quite a few changes to make.
> 
> I'm ready for review with an updated webrev, but thought first I'd have you 
> look at and comment on the output. Please see the attached file. It now 
> supports:
> 
>printing the hierarchy for a single class
>optionally including all subclasses of the specified class (-s)
>for each class, also list all of its interfaces (-i)
> 
> The hex value in the output is the address of the ClassLoaderData for the 
> ClassLoader of the class. I did not include the address of the Klass, but 
> could if you think it would be useful. Changing the format of what comes 
> after the classname is easy. Just let me know what you think is best.
> 
> I have not updated any other dcmds to be consistent with how classes are 
> uniquely identified. A separate bug should be filed for that. Actually I 
> thought one was, but I looked through this thread's history and could not 
> find mention of it. I also have not implemented the reverse hierarchy dcmd. 
> JDK-8068830 has already been filed for that, but there are no plans to work 
> on it in the near term.
> 
> thanks,
> 
> Chris

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

[Hmm. My previous email somehow included the attachment with the dcmd 
output, but not the body of the message, so here it is again.]


Hey Folks,

Sorry about the delay in getting the next webrev for this out. I was 
sidetracked by a few other things, including being out sick of almost a 
week, and there were also quite a few changes to make.


I'm ready for review with an updated webrev, but thought first I'd have 
you look at and comment on the output. Please see the attached file. It 
now supports:


printing the hierarchy for a single class
optionally including all subclasses of the specified class (-s)
for each class, also list all of its interfaces (-i)

The hex value in the output is the address of the ClassLoaderData for 
the ClassLoader of the class. I did not include the address of the 
Klass, but could if you think it would be useful. Changing the format of 
what comes after the classname is easy. Just let me know what you think 
is best.


I have not updated any other dcmds to be consistent with how classes are 
uniquely identified. A separate bug should be filed for that. Actually I 
thought one was, but I looked through this thread's history and could 
not find mention of it. I also have not implemented the reverse 
hierarchy dcmd. JDK-8068830 has already been filed for that, but there 
are no plans to work on it in the near term.


thanks,

Chris

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
[Hmm. My previous email somehow included the attachment, but not the
body of the message, so here it is again.]

Hey Folks,

Sorry about the delay in getting the next webrev for this out. I was
sidetracked by a few other things, including being out sick of
almost a week, and there were also quite a few changes to make.

I'm ready for review with an updated webrev, but thought first I'd
have you look at and comment on the output. Please see the attached
file. It now supports:

  printing the hierarchy for a single class
  optionally including all subclasses of the specified class
(-s)
  for each class, also list all of its interfaces (-i)

The hex value in the output is the address of the ClassLoaderData
for the ClassLoader of the class. I did not include the address of
the Klass, but could if you think it would be useful. Changing the
format of what comes after the classname is easy. Just let me know
what you think is best.

I have not updated any other dcmds to be consistent with how classes
are uniquely identified. A separate bug should be filed for that.
Actually I thought one was, but I looked through this thread's
history and could not find mention of it. I also have not
implemented the reverse hierarchy dcmd. JDK-8068830
has already been filed for that, but there are no plans to work on
it in the near term.

thanks,

Chris
  

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
Hey Folks,
  
  Sorry about the delay in getting the next webrev for this out. I
  was sidetracked by a few other things, including being out sick of
  almost a week, and there were also quite a few changes to make.
  
  I'm ready for review with an updated webrev, but thought first I'd
  have you look at and comment on the output. Please see the
  attached file. It now supports:
  
printing the hierarchy for a single class
optionally including all subclasses of the specified class
  (-s)
for each class, also list all of its interfaces (-i)
  
  The hex value in the output is the address of the ClassLoaderData
  for the ClassLoader of the class. I did not include the address of
  the Klass, but could if you think it would be useful. Changing the
  format of what comes after the classname is easy. Just let me know
  what you think is best.
  
  I have not updated any other dcmds to be consistent with how
  classes are uniquely identified. A separate bug should be filed
  for that. Actually I thought one was, but I looked through this
  thread's history and could not find mention of it. I also have not
  implemented the reverse hierarchy dcmd.
  
  JDK-8068830
  has already been filed for that, but there are no plans to work on
  it in the near term.
  
  thanks,
  
  Chris
  
  On 1/7/15 3:29 PM, Chris Plummer wrote:

Hi,
  
  
  Please review the following changes for the addition of the
  VM.class_hierarchy DCMD. Please read the bug first for some
  background information.
  
  
  Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
  
  Bug: https://bugs.openjdk.java.net/browse/JDK-8054888
  
  
  I expect there will be further restructuring or additional feature
  work. More discussion on that below. I'm not sure if that
  additional work will be done later with a separately bug filed or
  with this initial commit. That's one thing I want to work out with
  this review.
  
  
  Currently the bulk of the DCMD is implemented in
  heapInspection.cpp. The  main purpose of this file is to implement
  the GC.class_stats and GC.class_histogram DCMDs. Both of them
  require walking the java heap to count live objects of each type,
  thus the name "heapInspection.cpp". This new VM.class_hierarchy
  DCMD does not require walking the heap, but is implemented in this
  file because it leverages the existing KlassInfoTable and related
  classes (KlassInfoEntry, KlassInfoBucket, and KlassClosure).
  
  
  KlassInfoTable makes it easy to build a database of all loaded
  classes, save additional info gathered for each class, iterate
  over them quickly, and also do quick lookups. This exactly what I
  needed for this DCMD, thus the reuse. There is some downside to
  this. For starters, heapInspection.cpp is not the proper place for
  a DCMD that has nothing to do with heap inspection. Also,
  KlassInfoEntry is being overloaded now to support 3 different
  DCMDs, as is KlassInfoTable. As a result each has a few fields and
  methods that are not used for all 3 DCMDs. Some subclassing might
  be in order here, but I'm not sure if it's worth it. Opinions
  welcomed. If I am going to refactor, I would prefer that be done
  as a next step so I'm not disturbing the existing DCMDs with this
  first implementation.
  
  
  I added some comments to code only used for GC.class_stats and
  GC.class_histogram. I did this when trying to figure them out so I
  could better understand how to implement VM.class_hierarchy. I can
  take them out if you think they are not appropriate for this
  commit.
  
  
  One other item I like to discuss is whether it is worth adding a
  class name argument to this DCMD. That would cause just the
  superclasses and subclasses of the named class to be printed. If
  you think that is useful, I think it can be added without too much
  trouble.
  
  
  At the moment not much testing has been done other than running
  the DCMD and looking at the output. I'll do more once it's clear
  the code has "settled". I would like to know if there are any
  existing tests for GC.class_stats and GC.class_histogram (there
  are none in the "test" directory). If so, possibly one could serve
  as the basis for a new test for VM.class_hierarchy.
  
  
  thanks,
  
  
  Chris
  


  

$> jcmd NeverExit help VM.class_hierarchy
25101:
VM.class_hierarchy
Print a list of all loaded classes, indented to show the class hiearchy.

Impact: Medium: Depends on number of loaded classes.

Syntax : VM.class_hierarchy [options] []

Arguments:
classname : [op

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Karen Kinnear]

Chris,

I filed JDK-8068830 to capture the request I had to print the super types of
a given class - so you don't have to deal with that as part of this exercise.

thanks,
Karen

On Jan 9, 2015, at 12:53 PM, Karen Kinnear wrote:

> Thanks Frederic for suggesting two different dcmds - they could
> share a lot of the code logic.
> 
> If folks generally prefer these as separate dcmds - I can file an
> rfe to add the inverted one - i.e. start at a given class/interface and
> tell me its supertypes.
> 
> thanks,
> Karen
> 
> On Jan 9, 2015, at 3:53 AM, Frederic Parain wrote:
> 
>> 
>> 
>> On 01/08/2015 10:29 PM, Chris Plummer wrote:
>>> Hi Karen,
>>> 
>>> Comments inline.
>>> 
>>> On 1/8/15 8:07 AM, Karen Kinnear wrote:
 Chris,
 
 Thank you for doing this. I had a couple of questions/comments.
 
 I like your idea of being able to start with a specific class to show
 all subclasses of.
>>> Ok. I'll add that.
 
 I think the way I read the code this shows the superclass hierarchy,
 not the superinterfaces. With the addition
 of default methods in interfaces, I think we have increased the value
 in seeing superinterfaces.
>>> It does include interfaces in the output, but not as part of any class
>>> hierarchy. Interfaces are just shown as regular classes that inherit
>>> from Object. This is the case if one interface extends another, I
>>> suppose because "extends" is just interpreted as "implements" in this case.
 
 So what I personally would find useful would be to be able to start
 with a specific class and
 find the superclasses and superinterfaces of that class - for the
 debugging I do, I usually am
 trying to look up and need both sets of information. Today if you run
 -XX:+TraceDefaultMethods
 there is one sample way to display all the supertypes of a single
 type, all the way up. I don't know the
 best way to make that consistent with the current output approach,
 e.g. using the |- syntax.
 
 e.g.
 Class java.util.Arrays$ArrayList requires default method processing
 java/util/Arrays$ArrayList
  java/util/AbstractList
java/util/AbstractCollection
  java/lang/Object
  java/util/Collection
java/lang/Object
java/lang/Iterable
  java/lang/Object
java/util/List
  java/lang/Object
  java/util/Collection
java/lang/Object
java/lang/Iterable
  java/lang/Object
  java/util/RandomAccess
java/lang/Object
  java/io/Serializable
java/lang/Object
 
 Do you think there could be value to others in the ability to walk up
 the hierarchy as well as to
 see superclasses and superinterfaces at least from that perspective?
>>> This is a inverted from how my dcmd prints the hierarchy, plus also
>>> includes interfaces. Inverting the hierarchy means a class is listed
>>> with every subclass of the class, which I don't think is desirable.
>>> Including interfaces has the same issue, but introduces a new issue even
>>> when not inverting the hierarchy. The same interface can be in more than
>>> one location in the hierarchy, so there is no avoiding printing it more
>>> than once, so we need to decide how to best include interfaces in the
>>> output.
>> 
>> It seems to me that we have two very different use cases here, each one
>> best served with a different output format:
>> 
>> 1 - Listing of all classes/interfaces hierarchy when the dcmd is
>>invoked without arguments:
>>   -> Chris' output format as described below (with interfaces)
>> 2 - Investigation on a particular class or interface when a class
>>or interface is passed in argument to the dcmd
>>   -> Karen's output format, much easier to work with to
>>  track default methods. Because the output is limited to the
>>  hierarchy from a single class, there's no class duplication
>>  in output (single parent class inheritance) and limited
>>  interfaces duplication.
>> 
>> If the implementations of the two features are too different, we could
>> consider having two different dcmds.
>> 
>> My 2 cents,
>> 
>> Fred
>> 
>>> The could just be a simple list right after the class that
>>> implements them:
>>> 
>>> java.lang.Object
>>> | ...
>>> |--MyBaseClass
>>> | |  implements -> MyInterface1
>>> | |  implements -> MyInterface2
>>> | |--MySubClass
>>> |  implements -> MyInterface1
>>> |  implements -> MyInterface2
>>> | ...
>>> |--MyInterface1
>>> |--MyInterface2
>>> 
>>> The "implements"  lines could be optional.
>>> 
>>> I know cvm would distinguish between interfaces the Class declared it
>>> implemented, and those it inherited from the interfaces it declared it
>>> implemented. This was necessary for reflection, and I think also to
>>> properly build up interfaces tables. I assume hotspot does something
>>> similar. Is there any need for this information to be 

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

> On 9 jan 2015, at 21:08, Chris Plummer  wrote:
> 
> On 1/9/15 4:38 AM, Staffan Larsen wrote:
>> It�s getting difficult to get all the information into the same output: 
>> hierarchy, interfaces, class loaders and modules. I took a stab at it and it 
>> could look like this:
>> 
>> java.lang.Object
>> |--java.io.Serializable (java.base, 0x0007c00375f8, iface)
>> |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
>> |--java.lang.Iterable (java.base, 0x0007c00375f8, iface)
>> | |--java.util.Collection (java.base, 0x0007c00375f8, iface)
>> | | |--java.util.List (java.base, 0x0007c00375f8, iface)
>> |--java.util.AbstractCollection  (java.base, 0x0007c00375f8)
>> | |  implements java.util.Collection
>> | |  implements java.lang.Iterable
>> | |--java.util.AbstractList (java.base, 0x0007c00375f8)
>> | |implements java.util.List
>> | | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8)
>> | | |implements java.io.Serializable
>> | | |implements java.util.RandomAccess
> For the most part I like this. The one part I'm not too fond of is hex long 
> which I assume represents the ClassLoader. Maybe we could just use a simple 
> index, and at the end of the dump include a table of the referenced 
> ClassLoaders. Possibly just hijack what VM.classloader_stats outputs, but 
> include the simple index in the first column.
>>  
>> 
>> But this is pushing what can be visualized in one place.
>> 
>> We will need to add module and class loader information somehow to all the 
>> Diagnostic Commands that list classes. In addition we will need a way to see 
>> how modules relate to one another. Perhaps it isn�t possible to have all the 
>> information in one place, but instead make it possible to cross reference 
>> between different diagnostic commands. For example, GC.class_stats could 
>> have the module and class loader information, and GC.class_hierarchy would 
>> not have to include it. What is missing from making that possible is a 
>> unique way of identifying a class (since the name is not unique). All output 
>> would need to include that unique identifier and it would be possible to 
>> cross reference. The identifier has to be stable during a JVM run, but not 
>> between runs.
> I don't know if there is some sort of simple stable ID for a class other than 
> the address of the Klass instance. And of course this assumes there isn't any 
> class unloading going on that could result in reuse of the address. Is that 
> acceptable?

Yeah, I was thinking about the Klass*, but as you say it isn’t guaranteed to be 
unique over time. Karen suggested using {class loader, class} which has the 
same problem, but includes more useful information, so I think that is 
preferable. (Ideally we should have a unique id assigned to classes and class 
loaders when they are created).

>> 
>> The above would then become:
>> 
>> java.lang.Object/0x12345600
>> |--java.io.Serializable/0x12345601
>> |--java.util.RandomAccess/0x12345602
>> |--java.lang.Iterable/0x12345603
>> | |--java.util.Collection/0x12345604
>> | | |--java.util.List/0x12345605
>> |--java.util.AbstractCollection/0x12345606
>> | |  implements java.util.Collection/0x12345604
>> | |  implements java.lang.Iterable/0x12345603
>> | |--java.util.AbstractList/0x12345607
>> | |implements java.util.List/0x12345605
>> | | |--java.util.Arrays$ArrayList/0x12345608
>> | | |implements java.io.Serializable/0x12345601
>> | | |implements java.util.RandomAccess/0x12345602
>> 
>> With additions to GC.class_stats:
>> 
>> Index Super ClassLoaderClassName
>> 1-1 0x0007c0034c48 java.lang.Object/0x12345600
>> 2 1 0x0007c0034c48 java.util.List/0x12345605
>> 331 0x0007c0034c48 java.util.AbstractList/0x12345607
>> 
>> And GC.class_histogram:
>> 
>>  num #instances #bytes  class name
>> --
>>1:   945 117736  java.lang.Object/0x12345600
>>2:   442  50352  java.util.List/0x12345605
>>3:   499  25288  java.util.AbstractList/0x12345607
> Would it be better for the class identifier to be in a separate column from 
> the class name, or is combining them intentional? In any case, it's easy 
> enough to add to all of the above assuming we are talking about an existing 
> value such as the address of the Klass instance.

Perhaps better to separate them, yes.

/Staffan

> 
> Chris
>> 
>> 
>> 
>> /Staffan
>> 
>>> On 9 jan 2015, at 09:53, Frederic Parain >> > wrote:
>>> 
>>> 
>>> 
>>> On 01/08/2015 10:29 PM, Chris Plummer wrote:
 Hi Karen,
 
 Comments inline.
 
 On 1/8/15 8:07 AM, Karen Kinnear wrote:
> Chris,
> 
> Thank you for doing this. I had a couple of questions/comments.
> 
> I like your idea of being able to start with a specific class to show
> all subclasses of.
 Ok. I'll add that.
> 
> 

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
On 1/9/15 4:38 AM, Staffan Larsen
  wrote:


  
  It�s getting difficult to get all the information into the same
  output: hierarchy, interfaces, class loaders and modules. I took a
  stab at it and it could look like this:
  
  
  
java.lang.Object
|--java.io.Serializable
(java.base, 0x0007c00375f8, iface)
|--java.util.RandomAccess
(java.base, 0x0007c00375f8, iface)
|--java.lang.Iterable
(java.base, 0x0007c00375f8, iface)
|
|--java.util.Collection (java.base, 0x0007c00375f8,
iface)
| |
|--java.util.List (java.base, 0x0007c00375f8, iface)
|--java.util.AbstractCollection
 (java.base, 0x0007c00375f8)
| |  implements
java.util.Collection
| |  implements java.lang.Iterable
|
|--java.util.AbstractList (java.base, 0x0007c00375f8)
| |    implements java.util.List
| |
|--java.util.Arrays$ArrayList (java.base,
0x0007c00375f8)
| | |    implements java.io.Serializable
| | |    implements java.util.RandomAccess
  

For the most part I like this. The one part I'm
  not too fond of is hex long which I assume represents the
  ClassLoader. Maybe we could just use a simple index, and at the
  end of the dump include a table of the referenced ClassLoaders.
  Possibly just hijack what VM.classloader_stats outputs, but
  include the simple index in the first column.

  
 


But this is pushing what can be visualized in one
  place.


We will need to add module and class loader
  information somehow to all the Diagnostic Commands that list
  classes. In addition we will need a way to see how modules
  relate to one another. Perhaps it isn�t possible to have all
  the information in one place, but instead make it possible to
  cross reference between different diagnostic commands. For
  example, GC.class_stats could have the module and class loader
  information, and GC.class_hierarchy would not have to include
  it. What is missing from making that possible is a unique way
  of identifying a class (since the name is not unique). All
  output would need to include that unique identifier and it
  would be possible to cross reference. The identifier has to be
  stable during a JVM run, but not between runs.
  

I don't know if there is some sort of simple stable ID for a class
other than the address of the Klass instance. And of course this
assumes there isn't any class unloading going on that could result
in reuse of the address. Is that acceptable?

  


The above would then become:



  java.lang.Object/0x12345600
  |--java.io.Serializable/0x12345601
  |--java.util.RandomAccess/0x12345602
  |--java.lang.Iterable/0x12345603
  |
  |--java.util.Collection/0x12345604
  | |
  |--java.util.List/0x12345605
  |--java.util.AbstractCollection/0x12345606
  | |  implements
  java.util.Collection/0x12345604
  | |  implements java.lang.Iterable/0x12345603
  |
  |--java.util.AbstractList/0x12345607
  | |    implements java.util.List/0x12345605
  | |
  |--java.util.Arrays$ArrayList/0x12345608
  | | |    implements java.io.Serializable/0x12345601
  | | |    implements java.util.RandomAccess/0x12345602


  

  With additions to GC.class_stats:




  Index Super
  ClassLoader        ClassName
      1    -1
  0x0007c0034c48 java.lang.Object/0x12345600
  
    2     1
0x0007c0034c48 java.util.List/0x12345605
  
  
    3  
 31 0x0007c0034c48 java.util.AbstractList/0x12345607
  

  


  

  
  


  And GC.class_histogram:




   num    
  #instances         #bytes  class name
  --
     1:          
  945         117736  java.lang.Object/0x12345600
     2:          
  442          50352  java.util.List/0x12345605
     3:          
  499          25288  java.util.AbstractList/0x12345607

  

Would it be better for the class identifier to be in a separate
column from the class name, or is combining them intentional? In any
  

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
On 1/9/15 4:38 AM, Staffan Larsen
  wrote:


  
  It�s getting difficult to get all the information into the same
  output: hierarchy, interfaces, class loaders and modules. I took a
  stab at it and it could look like this:
  
  
  
java.lang.Object
|--java.io.Serializable
(java.base, 0x0007c00375f8, iface)
|--java.util.RandomAccess
(java.base, 0x0007c00375f8, iface)
|--java.lang.Iterable
(java.base, 0x0007c00375f8, iface)
|
|--java.util.Collection (java.base, 0x0007c00375f8,
iface)
| |
|--java.util.List (java.base, 0x0007c00375f8, iface)
|--java.util.AbstractCollection
 (java.base, 0x0007c00375f8)
| |  implements
java.util.Collection
| |  implements java.lang.Iterable
|
|--java.util.AbstractList (java.base, 0x0007c00375f8)
| |    implements java.util.List
| |
|--java.util.Arrays$ArrayList (java.base,
0x0007c00375f8)
| | |    implements java.io.Serializable
| | |    implements java.util.RandomAccess
  

For the most part I like this. The one part I'm
  not too fond of is hex long which I assume represents the
  ClassLoader. Maybe we could just use a simple index, and at the
  end of the dump include a table of the referenced ClassLoaders.
  Possibly just hijack what VM.classloader_stats outputs, but
  include the simple index in the first column.

  
 


But this is pushing what can be visualized in one
  place.


We will need to add module and class loader
  information somehow to all the Diagnostic Commands that list
  classes. In addition we will need a way to see how modules
  relate to one another. Perhaps it isn�t possible to have all
  the information in one place, but instead make it possible to
  cross reference between different diagnostic commands. For
  example, GC.class_stats could have the module and class loader
  information, and GC.class_hierarchy would not have to include
  it. What is missing from making that possible is a unique way
  of identifying a class (since the name is not unique). All
  output would need to include that unique identifier and it
  would be possible to cross reference. The identifier has to be
  stable during a JVM run, but not between runs.
  

I don't know if there is some sort of simple stable ID for a class
other than the address of the Klass instance. And of course this
assumes there isn't any class unloading going on that could result
in reuse of the address. Is that acceptable?

  


The above would then become:



  java.lang.Object/0x12345600
  |--java.io.Serializable/0x12345601
  |--java.util.RandomAccess/0x12345602
  |--java.lang.Iterable/0x12345603
  |
  |--java.util.Collection/0x12345604
  | |
  |--java.util.List/0x12345605
  |--java.util.AbstractCollection/0x12345606
  | |  implements
  java.util.Collection/0x12345604
  | |  implements java.lang.Iterable/0x12345603
  |
  |--java.util.AbstractList/0x12345607
  | |    implements java.util.List/0x12345605
  | |
  |--java.util.Arrays$ArrayList/0x12345608
  | | |    implements java.io.Serializable/0x12345601
  | | |    implements java.util.RandomAccess/0x12345602


  

  With additions to GC.class_stats:




  Index Super
  ClassLoader        ClassName
      1    -1
  0x0007c0034c48 java.lang.Object/0x12345600
  
    2     1
0x0007c0034c48 java.util.List/0x12345605
  
  
    3  
 31 0x0007c0034c48 java.util.AbstractList/0x12345607
  

  


  

  
  


  And GC.class_histogram:




   num    
  #instances         #bytes  class name
  --
     1:          
  945         117736  java.lang.Object/0x12345600
     2:          
  442          50352  java.util.List/0x12345605
     3:          
  499          25288  java.util.AbstractList/0x12345607

  

Would it be better for the class identifier to be in a separate
column from the class name, or is combining them intentional? In any
  

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

> On 9 jan 2015, at 18:49, Karen Kinnear  wrote:
> 
> Staffan,
> 
> On Jan 9, 2015, at 7:38 AM, Staffan Larsen wrote:
> 
>> It’s getting difficult to get all the information into the same output: 
>> hierarchy, interfaces, class loaders and modules. I took a stab at it and it 
>> could look like this:
>> 
>> java.lang.Object
>> |--java.io.Serializable (java.base, 0x0007c00375f8, iface)
>> |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
>> |--java.lang.Iterable (java.base, 0x0007c00375f8, iface)
>> | |--java.util.Collection (java.base, 0x0007c00375f8, iface)
>> | | |--java.util.List (java.base, 0x0007c00375f8, iface)
>> |--java.util.AbstractCollection  (java.base, 0x0007c00375f8)
>> | |  implements java.util.Collection
>> | |  implements java.lang.Iterable
>> | |--java.util.AbstractList (java.base, 0x0007c00375f8)
>> | |implements java.util.List
>> | | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8)
>> | | |implements java.io.Serializable
>> | | |implements java.util.RandomAccess
>>  
>> 
>> But this is pushing what can be visualized in one place.
>> 
>> We will need to add module and class loader information somehow to all the 
>> Diagnostic Commands that list classes. In addition we will need a way to see 
>> how modules relate to one another. Perhaps it isn’t possible to have all the 
>> information in one place, but instead make it possible to cross reference 
>> between different diagnostic commands. For example, GC.class_stats could 
>> have the module and class loader information, and GC.class_hierarchy would 
>> not have to include it. What is missing from making that possible is a 
>> unique way of identifying a class (since the name is not unique). All output 
>> would need to include that unique identifier and it would be possible to 
>> cross reference. The identifier has to be stable during a JVM run, but not 
>> between runs.
> I like where you are going with this. Since a given module is within a class 
> loader, putting the the loader/module/class  information in class_stats would 
> be helpful and then having
> other dcmds just need the class/class loader pair for uniqueness. 
> 
> Not sure what you are using below for unique identifier.
> I would be tempted to use the class loader hex value since otherwise you are 
> introducing hex values that have no meaning other than as
> cross-references. And the class/class loader pair is guaranteed uniqueness.

I was actually thinking of a unique identifier for a class (the Klass* for 
example), not a {class name, class loader} combo, but as you say, the {class 
name, class loader} combo is also unique and gives more useful information.


> For any class loader you could list its hex value thereby giving the 
> information in a single command that gives meaning to the loader value. 
> 
> java.lang.Object/null
> ...
> |-sun.misc.Launcher$AppClassLoader/null (0xyyy)
> |-myapp/0xyyy
> 
> Just a thought. If you are not enthused - I am ok with your proposal.

This looks good to me.

/Staffan

> 
> thanks,
> Karen
> 
>> 
>> The above would then become:
>> 
>> java.lang.Object/0x12345600
>> |--java.io.Serializable/0x12345601
>> |--java.util.RandomAccess/0x12345602
>> |--java.lang.Iterable/0x12345603
>> | |--java.util.Collection/0x12345604
>> | | |--java.util.List/0x12345605
>> |--java.util.AbstractCollection/0x12345606
>> | |  implements java.util.Collection/0x12345604
>> | |  implements java.lang.Iterable/0x12345603
>> | |--java.util.AbstractList/0x12345607
>> | |implements java.util.List/0x12345605
>> | | |--java.util.Arrays$ArrayList/0x12345608
>> | | |implements java.io.Serializable/0x12345601
>> | | |implements java.util.RandomAccess/0x12345602
>> 
>> With additions to GC.class_stats:
>> 
>> Index Super ClassLoaderClassName
>> 1-1 0x0007c0034c48 java.lang.Object/0x12345600
>> 2 1 0x0007c0034c48 java.util.List/0x12345605
>> 331 0x0007c0034c48 java.util.AbstractList/0x12345607
>> 
>> And GC.class_histogram:
>> 
>>  num #instances #bytes  class name
>> --
>>1:   945 117736  java.lang.Object/0x12345600
>>2:   442  50352  java.util.List/0x12345605
>>3:   499  25288  java.util.AbstractList/0x12345607
>> 
>> 
>> 
>> /Staffan
>> 
>>> On 9 jan 2015, at 09:53, Frederic Parain >> > wrote:
>>> 
>>> 
>>> 
>>> On 01/08/2015 10:29 PM, Chris Plummer wrote:
 Hi Karen,
 
 Comments inline.
 
 On 1/8/15 8:07 AM, Karen Kinnear wrote:
> Chris,
> 
> Thank you for doing this. I had a couple of questions/comments.
> 
> I like your idea of being able to start with a specific class to show
> all subclasses of.
 Ok. I'll add that.
> 
> I think the way I read the code this shows the superclass hierarchy,
> not the superinterfaces. With the addition
>>

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Karen Kinnear]

Thanks Frederic for suggesting two different dcmds - they could
share a lot of the code logic.

If folks generally prefer these as separate dcmds - I can file an
rfe to add the inverted one - i.e. start at a given class/interface and
tell me its supertypes.

thanks,
Karen

On Jan 9, 2015, at 3:53 AM, Frederic Parain wrote:

> 
> 
> On 01/08/2015 10:29 PM, Chris Plummer wrote:
>> Hi Karen,
>> 
>> Comments inline.
>> 
>> On 1/8/15 8:07 AM, Karen Kinnear wrote:
>>> Chris,
>>> 
>>> Thank you for doing this. I had a couple of questions/comments.
>>> 
>>> I like your idea of being able to start with a specific class to show
>>> all subclasses of.
>> Ok. I'll add that.
>>> 
>>> I think the way I read the code this shows the superclass hierarchy,
>>> not the superinterfaces. With the addition
>>> of default methods in interfaces, I think we have increased the value
>>> in seeing superinterfaces.
>> It does include interfaces in the output, but not as part of any class
>> hierarchy. Interfaces are just shown as regular classes that inherit
>> from Object. This is the case if one interface extends another, I
>> suppose because "extends" is just interpreted as "implements" in this case.
>>> 
>>> So what I personally would find useful would be to be able to start
>>> with a specific class and
>>> find the superclasses and superinterfaces of that class - for the
>>> debugging I do, I usually am
>>> trying to look up and need both sets of information. Today if you run
>>> -XX:+TraceDefaultMethods
>>> there is one sample way to display all the supertypes of a single
>>> type, all the way up. I don't know the
>>> best way to make that consistent with the current output approach,
>>> e.g. using the |- syntax.
>>> 
>>> e.g.
>>> Class java.util.Arrays$ArrayList requires default method processing
>>> java/util/Arrays$ArrayList
>>>   java/util/AbstractList
>>> java/util/AbstractCollection
>>>   java/lang/Object
>>>   java/util/Collection
>>> java/lang/Object
>>> java/lang/Iterable
>>>   java/lang/Object
>>> java/util/List
>>>   java/lang/Object
>>>   java/util/Collection
>>> java/lang/Object
>>> java/lang/Iterable
>>>   java/lang/Object
>>>   java/util/RandomAccess
>>> java/lang/Object
>>>   java/io/Serializable
>>> java/lang/Object
>>> 
>>> Do you think there could be value to others in the ability to walk up
>>> the hierarchy as well as to
>>> see superclasses and superinterfaces at least from that perspective?
>> This is a inverted from how my dcmd prints the hierarchy, plus also
>> includes interfaces. Inverting the hierarchy means a class is listed
>> with every subclass of the class, which I don't think is desirable.
>> Including interfaces has the same issue, but introduces a new issue even
>> when not inverting the hierarchy. The same interface can be in more than
>> one location in the hierarchy, so there is no avoiding printing it more
>> than once, so we need to decide how to best include interfaces in the
>> output.
> 
> It seems to me that we have two very different use cases here, each one
> best served with a different output format:
> 
> 1 - Listing of all classes/interfaces hierarchy when the dcmd is
> invoked without arguments:
>-> Chris' output format as described below (with interfaces)
> 2 - Investigation on a particular class or interface when a class
> or interface is passed in argument to the dcmd
>-> Karen's output format, much easier to work with to
>   track default methods. Because the output is limited to the
>   hierarchy from a single class, there's no class duplication
>   in output (single parent class inheritance) and limited
>   interfaces duplication.
> 
> If the implementations of the two features are too different, we could
> consider having two different dcmds.
> 
> My 2 cents,
> 
> Fred
> 
>> The could just be a simple list right after the class that
>> implements them:
>> 
>> java.lang.Object
>> | ...
>> |--MyBaseClass
>> | |  implements -> MyInterface1
>> | |  implements -> MyInterface2
>> | |--MySubClass
>> |  implements -> MyInterface1
>> |  implements -> MyInterface2
>> | ...
>> |--MyInterface1
>> |--MyInterface2
>> 
>> The "implements"  lines could be optional.
>> 
>> I know cvm would distinguish between interfaces the Class declared it
>> implemented, and those it inherited from the interfaces it declared it
>> implemented. This was necessary for reflection, and I think also to
>> properly build up interfaces tables. I assume hotspot does something
>> similar. Is there any need for this information to be conveyed in the
>> above output, or just list out every interface implemented, and not
>> worry about how the class acquired it.
>>> Is there value in printing the defining class loader for each class -
>>> maybe optionally?
>> This is already available with GC.class_stats, although not in the
>> default output. I suppose the reality is th

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Karen Kinnear]

Staffan,

On Jan 9, 2015, at 7:38 AM, Staffan Larsen wrote:

> It’s getting difficult to get all the information into the same output: 
> hierarchy, interfaces, class loaders and modules. I took a stab at it and it 
> could look like this:
> 
> java.lang.Object
> |--java.io.Serializable (java.base, 0x0007c00375f8, iface)
> |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
> |--java.lang.Iterable (java.base, 0x0007c00375f8, iface)
> | |--java.util.Collection (java.base, 0x0007c00375f8, iface)
> | | |--java.util.List (java.base, 0x0007c00375f8, iface)
> |--java.util.AbstractCollection  (java.base, 0x0007c00375f8)
> | |  implements java.util.Collection
> | |  implements java.lang.Iterable
> | |--java.util.AbstractList (java.base, 0x0007c00375f8)
> | |implements java.util.List
> | | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8)
> | | |implements java.io.Serializable
> | | |implements java.util.RandomAccess
>  
> 
> But this is pushing what can be visualized in one place.
> 
> We will need to add module and class loader information somehow to all the 
> Diagnostic Commands that list classes. In addition we will need a way to see 
> how modules relate to one another. Perhaps it isn’t possible to have all the 
> information in one place, but instead make it possible to cross reference 
> between different diagnostic commands. For example, GC.class_stats could have 
> the module and class loader information, and GC.class_hierarchy would not 
> have to include it. What is missing from making that possible is a unique way 
> of identifying a class (since the name is not unique). All output would need 
> to include that unique identifier and it would be possible to cross 
> reference. The identifier has to be stable during a JVM run, but not between 
> runs.
I like where you are going with this. Since a given module is within a class 
loader, putting the the loader/module/class  information in class_stats would 
be helpful and then having
other dcmds just need the class/class loader pair for uniqueness. 

Not sure what you are using below for unique identifier.
I would be tempted to use the class loader hex value since otherwise you are 
introducing hex values that have no meaning other than as
cross-references. And the class/class loader pair is guaranteed uniqueness.
For any class loader you could list its hex value thereby giving the 
information in a single command that gives meaning to the loader value. 

java.lang.Object/null
...
|-sun.misc.Launcher$AppClassLoader/null (0xyyy)
|-myapp/0xyyy

Just a thought. If you are not enthused - I am ok with your proposal.

thanks,
Karen

> 
> The above would then become:
> 
> java.lang.Object/0x12345600
> |--java.io.Serializable/0x12345601
> |--java.util.RandomAccess/0x12345602
> |--java.lang.Iterable/0x12345603
> | |--java.util.Collection/0x12345604
> | | |--java.util.List/0x12345605
> |--java.util.AbstractCollection/0x12345606
> | |  implements java.util.Collection/0x12345604
> | |  implements java.lang.Iterable/0x12345603
> | |--java.util.AbstractList/0x12345607
> | |implements java.util.List/0x12345605
> | | |--java.util.Arrays$ArrayList/0x12345608
> | | |implements java.io.Serializable/0x12345601
> | | |implements java.util.RandomAccess/0x12345602
> 
> With additions to GC.class_stats:
> 
> Index Super ClassLoaderClassName
> 1-1 0x0007c0034c48 java.lang.Object/0x12345600
> 2 1 0x0007c0034c48 java.util.List/0x12345605
> 331 0x0007c0034c48 java.util.AbstractList/0x12345607
> 
> And GC.class_histogram:
> 
>  num #instances #bytes  class name
> --
>1:   945 117736  java.lang.Object/0x12345600
>2:   442  50352  java.util.List/0x12345605
>3:   499  25288  java.util.AbstractList/0x12345607
> 
> 
> 
> /Staffan
> 
>> On 9 jan 2015, at 09:53, Frederic Parain  wrote:
>> 
>> 
>> 
>> On 01/08/2015 10:29 PM, Chris Plummer wrote:
>>> Hi Karen,
>>> 
>>> Comments inline.
>>> 
>>> On 1/8/15 8:07 AM, Karen Kinnear wrote:
 Chris,
 
 Thank you for doing this. I had a couple of questions/comments.
 
 I like your idea of being able to start with a specific class to show
 all subclasses of.
>>> Ok. I'll add that.
 
 I think the way I read the code this shows the superclass hierarchy,
 not the superinterfaces. With the addition
 of default methods in interfaces, I think we have increased the value
 in seeing superinterfaces.
>>> It does include interfaces in the output, but not as part of any class
>>> hierarchy. Interfaces are just shown as regular classes that inherit
>>> from Object. This is the case if one interface extends another, I
>>> suppose because "extends" is just interpreted as "implements" in this case.
 
 So what I personally would find useful would be to be able to start
 with a specific

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Staffan Larsen]

It’s getting difficult to get all the information into the same output: 
hierarchy, interfaces, class loaders and modules. I took a stab at it and it 
could look like this:

java.lang.Object
|--java.io.Serializable (java.base, 0x0007c00375f8, iface)
|--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
|--java.lang.Iterable (java.base, 0x0007c00375f8, iface)
| |--java.util.Collection (java.base, 0x0007c00375f8, iface)
| | |--java.util.List (java.base, 0x0007c00375f8, iface)
|--java.util.AbstractCollection  (java.base, 0x0007c00375f8)
| |  implements java.util.Collection
| |  implements java.lang.Iterable
| |--java.util.AbstractList (java.base, 0x0007c00375f8)
| |implements java.util.List
| | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8)
| | |implements java.io.Serializable
| | |implements java.util.RandomAccess
 

But this is pushing what can be visualized in one place.

We will need to add module and class loader information somehow to all the 
Diagnostic Commands that list classes. In addition we will need a way to see 
how modules relate to one another. Perhaps it isn’t possible to have all the 
information in one place, but instead make it possible to cross reference 
between different diagnostic commands. For example, GC.class_stats could have 
the module and class loader information, and GC.class_hierarchy would not have 
to include it. What is missing from making that possible is a unique way of 
identifying a class (since the name is not unique). All output would need to 
include that unique identifier and it would be possible to cross reference. The 
identifier has to be stable during a JVM run, but not between runs.

The above would then become:

java.lang.Object/0x12345600
|--java.io.Serializable/0x12345601
|--java.util.RandomAccess/0x12345602
|--java.lang.Iterable/0x12345603
| |--java.util.Collection/0x12345604
| | |--java.util.List/0x12345605
|--java.util.AbstractCollection/0x12345606
| |  implements java.util.Collection/0x12345604
| |  implements java.lang.Iterable/0x12345603
| |--java.util.AbstractList/0x12345607
| |implements java.util.List/0x12345605
| | |--java.util.Arrays$ArrayList/0x12345608
| | |implements java.io.Serializable/0x12345601
| | |implements java.util.RandomAccess/0x12345602

With additions to GC.class_stats:

Index Super ClassLoaderClassName
1-1 0x0007c0034c48 java.lang.Object/0x12345600
2 1 0x0007c0034c48 java.util.List/0x12345605
331 0x0007c0034c48 java.util.AbstractList/0x12345607

And GC.class_histogram:

 num #instances #bytes  class name
--
   1:   945 117736  java.lang.Object/0x12345600
   2:   442  50352  java.util.List/0x12345605
   3:   499  25288  java.util.AbstractList/0x12345607



/Staffan

> On 9 jan 2015, at 09:53, Frederic Parain  wrote:
> 
> 
> 
> On 01/08/2015 10:29 PM, Chris Plummer wrote:
>> Hi Karen,
>> 
>> Comments inline.
>> 
>> On 1/8/15 8:07 AM, Karen Kinnear wrote:
>>> Chris,
>>> 
>>> Thank you for doing this. I had a couple of questions/comments.
>>> 
>>> I like your idea of being able to start with a specific class to show
>>> all subclasses of.
>> Ok. I'll add that.
>>> 
>>> I think the way I read the code this shows the superclass hierarchy,
>>> not the superinterfaces. With the addition
>>> of default methods in interfaces, I think we have increased the value
>>> in seeing superinterfaces.
>> It does include interfaces in the output, but not as part of any class
>> hierarchy. Interfaces are just shown as regular classes that inherit
>> from Object. This is the case if one interface extends another, I
>> suppose because "extends" is just interpreted as "implements" in this case.
>>> 
>>> So what I personally would find useful would be to be able to start
>>> with a specific class and
>>> find the superclasses and superinterfaces of that class - for the
>>> debugging I do, I usually am
>>> trying to look up and need both sets of information. Today if you run
>>> -XX:+TraceDefaultMethods
>>> there is one sample way to display all the supertypes of a single
>>> type, all the way up. I don't know the
>>> best way to make that consistent with the current output approach,
>>> e.g. using the |- syntax.
>>> 
>>> e.g.
>>> Class java.util.Arrays$ArrayList requires default method processing
>>> java/util/Arrays$ArrayList
>>>   java/util/AbstractList
>>> java/util/AbstractCollection
>>>   java/lang/Object
>>>   java/util/Collection
>>> java/lang/Object
>>> java/lang/Iterable
>>>   java/lang/Object
>>> java/util/List
>>>   java/lang/Object
>>>   java/util/Collection
>>> java/lang/Object
>>> java/lang/Iterable
>>>   java/lang/Object
>>>   java/util/RandomAccess
>>> java/lang/Object
>>>   java/io/Serializable
>>> java/lang/Object
>>> 
>>> Do you think there co

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Stefan Karlsson]

On 2015-01-08 20:15, Chris Plummer wrote:

Hi Stefan,

Comments inline below:

On 1/8/15 2:50 AM, Stefan Karlsson wrote:

Hi Chris,

On 2015-01-08 00:29, Chris Plummer wrote:

Hi,

Please review the following changes for the addition of the 
VM.class_hierarchy DCMD. Please read the bug first for some 
background information.


Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054888


This looks like a nice feature. I think your suggestion about 
supporting a class name argument as the root of the hierarchy would 
be a nice addition.



Some comments:

Why do you need / use the super_stack? To me it seems like you could 
simplify the could if you get rid of the super_stack and walk the 
Klass::super() chain instead.
The initial implementation actually printed the superclass index as 
the indentation, which required the super_stack, and I just ended up 
keeping it. It looks like I could get rid of all the superclass 
maintenance done in print_class_hierarchy()  if I walk Klass:super() 
in print_class(). I'll make that change.


Why did you add this side-effect to KlassInfoHisto::print_class_state?:

-  super_index = super_e->index();
+  e->set_super_index(super_e->index());


AFAICT, you are not using KlassInfoHisto::print_class_stats to 
implement the VM.class_hierarchy DCMD, right? Or am I missing 
something in your patch?
Originally I did overload print_class_stats to also handle 
VM.class_hierarchy, but in the end decided not to due to the overhead 
of it causing the java heap to be walked. So the above is a remnant, 
but is also consistent with how print_class_hierarchy() sets the 
super_index field of the KlassInfoEntry. However, it may not be 
necessary anymore to maintain the super_index if I make the change 
above to no longer maintain the super_stack. I'll look into that.




A side-note, if it were not for the AnonymousClasses (created by 
Unsafe_DefineAnonymousClass), then this could have be implemented 
with less resources by just walking the Klass::subclass() and 
Klass::next_sibling() links. Which means that you wouldn't have to 
use any of the classes or functionality in heapInspection.hpp/cpp. 
But the anonymous classes is unfortunately not present in the 
subclass/next_sibling hierarchy.
I didn't not realize subclass info was being maintained by Klass. 
Still had CVM stuck in my head, which does not do that. I'm not sure 
what the AnonymousClasses issue is. Can you explain more?


The AnonymousClasses are supposed to be lightweight classes used by 
JSR292. They have a different lifecycle than ordinary Klasses, and are 
not registered in some of the data structures where the ordinary Klasses 
are registered, and one example is the subclass/next_sibling tree. 
Because these classes treated differently, they are a constant source of 
bugs. If you visit Klasses by iterating over a data structure in the 
JVM, you need to know if the AnonymousClasses are present our not. Your 
current code is safe because you walk the ClassLoaderDataGraph, which 
contains the AnonymousKlasses. But a simplified version relying on the 
subclass/next_sibling tree would unfortunately be broken. The 
SystemDictionary is another place where the AnonymousClasses are not 
registered.


Note, that the term "anonymous class" is an overloaded term and I'm not 
referring to this kind of anonymous classes:

http://docs.oracle.com/javase/tutorial/java/javaOO/anonymousclasses.html




And some style comments:

http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/services/diagnosticCommand.cpp.frames.html 



Maybe it would be nice to move:

  66 #if INCLUDE_SERVICES
  67   DCmdFactory::register_DCmdFactory(new 
DCmdFactoryImpl(full_export, true, false));

  68 #endif

into the already existing INCLUDE_SERVICE block:

  55 #if INCLUDE_SERVICES // Heap dumping/inspection supported
  56   DCmdFactory::register_DCmdFactory(new 
DCmdFactoryImpl(DCmd_Source_Internal | 
DCmd_Source_AttachAPI, true, false));
  57   DCmdFactory::register_DCmdFactory(new 
DCmdFactoryImpl(full_export, true, false));
  58   DCmdFactory::register_DCmdFactory(new 
DCmdFactoryImpl(full_export, true, false));

  59 #endif // INCLUDE_SERVICES

Ok.



http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.hpp.frames.html 




I would prefer if you moved the following implementation to the cpp 
file, so that we can keep our includes in our hpp files to a minimal. 
That helps lowering our include complexity.


 218 inline void KlassInfoEntry::add_subclass(KlassInfoEntry* cie) {
 219   if (_subclasses == NULL) {
 220 _subclasses = new  (ResourceObj::C_HEAP, mtInternal) 
GrowableArray(4, true);

 221   }
 222   _subclasses->append(cie);
 223 }
 224
 225 inline KlassInfoEntry::~KlassInfoEntry() {
 226   if (_subclasses != NULL) {
 227 delete _subclasses;
 228   }
 229 }

I guess I'm a bit unclear on this, and had already

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Frederic Parain]

On 01/08/2015 10:29 PM, Chris Plummer wrote:

Hi Karen,

Comments inline.

On 1/8/15 8:07 AM, Karen Kinnear wrote:

Chris,

Thank you for doing this. I had a couple of questions/comments.

I like your idea of being able to start with a specific class to show
all subclasses of.

Ok. I'll add that.


I think the way I read the code this shows the superclass hierarchy,
not the superinterfaces. With the addition
of default methods in interfaces, I think we have increased the value
in seeing superinterfaces.

It does include interfaces in the output, but not as part of any class
hierarchy. Interfaces are just shown as regular classes that inherit
from Object. This is the case if one interface extends another, I
suppose because "extends" is just interpreted as "implements" in this case.


So what I personally would find useful would be to be able to start
with a specific class and
find the superclasses and superinterfaces of that class - for the
debugging I do, I usually am
trying to look up and need both sets of information. Today if you run
-XX:+TraceDefaultMethods
there is one sample way to display all the supertypes of a single
type, all the way up. I don't know the
best way to make that consistent with the current output approach,
e.g. using the |- syntax.

e.g.
Class java.util.Arrays$ArrayList requires default method processing
java/util/Arrays$ArrayList
   java/util/AbstractList
 java/util/AbstractCollection
   java/lang/Object
   java/util/Collection
 java/lang/Object
 java/lang/Iterable
   java/lang/Object
 java/util/List
   java/lang/Object
   java/util/Collection
 java/lang/Object
 java/lang/Iterable
   java/lang/Object
   java/util/RandomAccess
 java/lang/Object
   java/io/Serializable
 java/lang/Object

Do you think there could be value to others in the ability to walk up
the hierarchy as well as to
see superclasses and superinterfaces at least from that perspective?

This is a inverted from how my dcmd prints the hierarchy, plus also
includes interfaces. Inverting the hierarchy means a class is listed
with every subclass of the class, which I don't think is desirable.
Including interfaces has the same issue, but introduces a new issue even
when not inverting the hierarchy. The same interface can be in more than
one location in the hierarchy, so there is no avoiding printing it more
than once, so we need to decide how to best include interfaces in the
output.


It seems to me that we have two very different use cases here, each one
best served with a different output format:

 1 - Listing of all classes/interfaces hierarchy when the dcmd is
 invoked without arguments:
-> Chris' output format as described below (with interfaces)
 2 - Investigation on a particular class or interface when a class
 or interface is passed in argument to the dcmd
-> Karen's output format, much easier to work with to
   track default methods. Because the output is limited to the
   hierarchy from a single class, there's no class duplication
   in output (single parent class inheritance) and limited
   interfaces duplication.

If the implementations of the two features are too different, we could
consider having two different dcmds.

My 2 cents,

Fred


The could just be a simple list right after the class that
implements them:

java.lang.Object
| ...
|--MyBaseClass
| |  implements -> MyInterface1
| |  implements -> MyInterface2
| |--MySubClass
|  implements -> MyInterface1
|  implements -> MyInterface2
| ...
|--MyInterface1
|--MyInterface2

The "implements"  lines could be optional.

I know cvm would distinguish between interfaces the Class declared it
implemented, and those it inherited from the interfaces it declared it
implemented. This was necessary for reflection, and I think also to
properly build up interfaces tables. I assume hotspot does something
similar. Is there any need for this information to be conveyed in the
above output, or just list out every interface implemented, and not
worry about how the class acquired it.

Is there value in printing the defining class loader for each class -
maybe optionally?

This is already available with GC.class_stats, although not in the
default output. I suppose the reality is that it might be better handled
by this DCMD. The main puprose of GC.class_stats is to print statistics
(counts and sizes), so printing the ClassLoader name there is probably
not appropriate, but then it's not really appropriate for
VM.class_hierarchy either. I'm not sure how best to handle this. One or
both DCMDs possibly should be re-purposed and more clearly define what
type of data it displays.

If so, I'm wondering if there might be value in future for the jigsaw
project adding the module for each class - maybe optionally as well?

This relates to my above statement. We need to figure out what type of
data is useful, and which dcmds should handle them.

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

Hi Karen,

Comments inline.

On 1/8/15 8:07 AM, Karen Kinnear wrote:

Chris,

Thank you for doing this. I had a couple of questions/comments.

I like your idea of being able to start with a specific class to show all 
subclasses of.

Ok. I'll add that.


I think the way I read the code this shows the superclass hierarchy, not the 
superinterfaces. With the addition
of default methods in interfaces, I think we have increased the value in seeing 
superinterfaces.
It does include interfaces in the output, but not as part of any class 
hierarchy. Interfaces are just shown as regular classes that inherit 
from Object. This is the case if one interface extends another, I 
suppose because "extends" is just interpreted as "implements" in this case.


So what I personally would find useful would be to be able to start with a 
specific class and
find the superclasses and superinterfaces of that class - for the debugging I 
do, I usually am
trying to look up and need both sets of information. Today if you run 
-XX:+TraceDefaultMethods
there is one sample way to display all the supertypes of a single type, all the 
way up. I don't know the
best way to make that consistent with the current output approach, e.g. using 
the |- syntax.

e.g.
Class java.util.Arrays$ArrayList requires default method processing
java/util/Arrays$ArrayList
   java/util/AbstractList
 java/util/AbstractCollection
   java/lang/Object
   java/util/Collection
 java/lang/Object
 java/lang/Iterable
   java/lang/Object
 java/util/List
   java/lang/Object
   java/util/Collection
 java/lang/Object
 java/lang/Iterable
   java/lang/Object
   java/util/RandomAccess
 java/lang/Object
   java/io/Serializable
 java/lang/Object

Do you think there could be value to others in the ability to walk up the 
hierarchy as well as to
see superclasses and superinterfaces at least from that perspective?
This is a inverted from how my dcmd prints the hierarchy, plus also 
includes interfaces. Inverting the hierarchy means a class is listed 
with every subclass of the class, which I don't think is desirable. 
Including interfaces has the same issue, but introduces a new issue even 
when not inverting the hierarchy. The same interface can be in more than 
one location in the hierarchy, so there is no avoiding printing it more 
than once, so we need to decide how to best include interfaces in the 
output. The could just be a simple list right after the class that 
implements them:


java.lang.Object
| ...
|--MyBaseClass
| |  implements -> MyInterface1
| |  implements -> MyInterface2
| |--MySubClass
|  implements -> MyInterface1
|  implements -> MyInterface2
| ...
|--MyInterface1
|--MyInterface2

The "implements"  lines could be optional.

I know cvm would distinguish between interfaces the Class declared it 
implemented, and those it inherited from the interfaces it declared it 
implemented. This was necessary for reflection, and I think also to 
properly build up interfaces tables. I assume hotspot does something 
similar. Is there any need for this information to be conveyed in the 
above output, or just list out every interface implemented, and not 
worry about how the class acquired it.

Is there value in printing the defining class loader for each class - maybe 
optionally?
This is already available with GC.class_stats, although not in the 
default output. I suppose the reality is that it might be better handled 
by this DCMD. The main puprose of GC.class_stats is to print statistics 
(counts and sizes), so printing the ClassLoader name there is probably 
not appropriate, but then it's not really appropriate for 
VM.class_hierarchy either. I'm not sure how best to handle this. One or 
both DCMDs possibly should be re-purposed and more clearly define what 
type of data it displays.

If so, I'm wondering if there might be value in future for the jigsaw project 
adding the module for each class - maybe optionally as well?
This relates to my above statement. We need to figure out what type of 
data is useful, and which dcmds should handle them.

Love opinions on that  - especially from the serviceability folks

thanks,
Karen

Thanks for the input.

Chris



On Jan 7, 2015, at 6:29 PM, Chris Plummer wrote:


Hi,

Please review the following changes for the addition of the VM.class_hierarchy 
DCMD. Please read the bug first for some background information.

Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054888

I expect there will be further restructuring or additional feature work. More 
discussion on that below. I'm not sure if that additional work will be done 
later with a separately bug filed or with this initial commit. That's one thing 
I want to work out with this review.

Currently the bulk of the DCMD is implemented in heapInspection.cpp. The  main purpose of 
this file is to implement the GC.class_stats and GC.cl

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Chris Plummer]

  
  
Hi Stefan,
  
  Comments inline below:
  
  On 1/8/15 2:50 AM, Stefan Karlsson wrote:

Hi
  Chris,
  
  
  On 2015-01-08 00:29, Chris Plummer wrote:
  
  Hi,


Please review the following changes for the addition of the
VM.class_hierarchy DCMD. Please read the bug first for some
background information.


Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/

Bug: https://bugs.openjdk.java.net/browse/JDK-8054888

  
  
  This looks like a nice feature. I think your suggestion about
  supporting a class name argument as the root of the hierarchy
  would be a nice addition.
  
  
  
  Some comments:
  
  
  Why do you need / use the super_stack? To me it seems like you
  could simplify the could if you get rid of the super_stack and
  walk the Klass::super() chain instead.
  

The initial implementation actually printed the superclass index as
the indentation, which required the super_stack, and I just ended up
keeping it. It looks like I could get rid of all the superclass
maintenance done in

print_class_hierarchy()  if I walk Klass:super() in print_class().
I'll make that change.

  
  Why did you add this side-effect to
  KlassInfoHisto::print_class_state?:
  
  
  -  super_index = super_e->index();
  
  +  e->set_super_index(super_e->index());
  


  
  
  AFAICT, you are not using KlassInfoHisto::print_class_stats to
  implement the VM.class_hierarchy DCMD, right? Or am I missing
  something in your patch?
  

Originally I did overload print_class_stats to also handle
VM.class_hierarchy, but in the end decided not to due to the
overhead of it causing the java heap to be walked. So the above is a
remnant, but is also consistent with how print_class_hierarchy()
sets the super_index field of the KlassInfoEntry. However, it may
not be necessary anymore to maintain the super_index if I make the
change above to no longer maintain the super_stack. I'll look into
that.


  
  A side-note, if it were not for the AnonymousClasses (created by
  Unsafe_DefineAnonymousClass), then this could have be implemented
  with less resources by just walking the Klass::subclass() and
  Klass::next_sibling() links. Which means that you wouldn't have to
  use any of the classes or functionality in heapInspection.hpp/cpp.
  But the anonymous classes is unfortunately not present in the
  subclass/next_sibling hierarchy.
  

I didn't not realize subclass info was being maintained by Klass.
Still had CVM stuck in my head, which does not do that. I'm not sure
what the AnonymousClasses issue is. Can you explain more?

  
  
  And some style comments:
  
  
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/services/diagnosticCommand.cpp.frames.html
  
  
  Maybe it would be nice to move:
  
  
    66 #if INCLUDE_SERVICES
  
    67   DCmdFactory::register_DCmdFactory(new
  DCmdFactoryImpl(full_export, true,
  false));
  
    68 #endif
  
  
  into the already existing INCLUDE_SERVICE block:
  
  
    55 #if INCLUDE_SERVICES // Heap dumping/inspection supported
  
    56   DCmdFactory::register_DCmdFactory(new
  DCmdFactoryImpl(DCmd_Source_Internal |
  DCmd_Source_AttachAPI, true, false));
  
    57   DCmdFactory::register_DCmdFactory(new
  DCmdFactoryImpl(full_export, true,
  false));
  
    58   DCmdFactory::register_DCmdFactory(new
  DCmdFactoryImpl(full_export, true, false));
  
    59 #endif // INCLUDE_SERVICES
  

Ok.

  
  
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.hpp.frames.html
  
  
  
  I would prefer if you moved the following implementation to the
  cpp file, so that we can keep our includes in our hpp files to a
  minimal. That helps lowering our include complexity.
  
  
   218 inline void KlassInfoEntry::add_subclass(KlassInfoEntry* cie)
  {
  
   219   if (_subclasses == NULL) {
  
   220 _subclasses = new  (ResourceObj::C_HEAP, mtInternal)
  GrowableArray(4, true);
  
   221   }
  
   222   _subclasses->append(cie);
  
   223 }
  
   224
  
   225 inline KlassInfoEntry::~KlassInfoEntry() {
  
   226   if (_subclasses != NULL) {
  
   227 delete _subclasses;
  
   228   }
  
   229 }
  
  

I guess I'm a bit unclear on this, and had already pondered where
this code should go. I've seen methods

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Karen Kinnear]

Chris,

Thank you for doing this. I had a couple of questions/comments.

I like your idea of being able to start with a specific class to show all 
subclasses of.

I think the way I read the code this shows the superclass hierarchy, not the 
superinterfaces. With the addition
of default methods in interfaces, I think we have increased the value in seeing 
superinterfaces.

So what I personally would find useful would be to be able to start with a 
specific class and
find the superclasses and superinterfaces of that class - for the debugging I 
do, I usually am
trying to look up and need both sets of information. Today if you run 
-XX:+TraceDefaultMethods
there is one sample way to display all the supertypes of a single type, all the 
way up. I don't know the
best way to make that consistent with the current output approach, e.g. using 
the |- syntax.

e.g.
Class java.util.Arrays$ArrayList requires default method processing
java/util/Arrays$ArrayList
  java/util/AbstractList
java/util/AbstractCollection
  java/lang/Object
  java/util/Collection
java/lang/Object
java/lang/Iterable
  java/lang/Object
java/util/List
  java/lang/Object
  java/util/Collection
java/lang/Object
java/lang/Iterable
  java/lang/Object
  java/util/RandomAccess
java/lang/Object
  java/io/Serializable
java/lang/Object

Do you think there could be value to others in the ability to walk up the 
hierarchy as well as to
see superclasses and superinterfaces at least from that perspective? 

Is there value in printing the defining class loader for each class - maybe 
optionally?
If so, I'm wondering if there might be value in future for the jigsaw project 
adding the module for each class - maybe optionally as well?
Love opinions on that  - especially from the serviceability folks 

thanks,
Karen


On Jan 7, 2015, at 6:29 PM, Chris Plummer wrote:

> Hi,
> 
> Please review the following changes for the addition of the 
> VM.class_hierarchy DCMD. Please read the bug first for some background 
> information.
> 
> Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8054888
> 
> I expect there will be further restructuring or additional feature work. More 
> discussion on that below. I'm not sure if that additional work will be done 
> later with a separately bug filed or with this initial commit. That's one 
> thing I want to work out with this review.
> 
> Currently the bulk of the DCMD is implemented in heapInspection.cpp. The  
> main purpose of this file is to implement the GC.class_stats and 
> GC.class_histogram DCMDs. Both of them require walking the java heap to count 
> live objects of each type, thus the name "heapInspection.cpp". This new 
> VM.class_hierarchy DCMD does not require walking the heap, but is implemented 
> in this file because it leverages the existing KlassInfoTable and related 
> classes (KlassInfoEntry, KlassInfoBucket, and KlassClosure).
> 
> KlassInfoTable makes it easy to build a database of all loaded classes, save 
> additional info gathered for each class, iterate over them quickly, and also 
> do quick lookups. This exactly what I needed for this DCMD, thus the reuse. 
> There is some downside to this. For starters, heapInspection.cpp is not the 
> proper place for a DCMD that has nothing to do with heap inspection. Also, 
> KlassInfoEntry is being overloaded now to support 3 different DCMDs, as is 
> KlassInfoTable. As a result each has a few fields and methods that are not 
> used for all 3 DCMDs. Some subclassing might be in order here, but I'm not 
> sure if it's worth it. Opinions welcomed. If I am going to refactor, I would 
> prefer that be done as a next step so I'm not disturbing the existing DCMDs 
> with this first implementation.
> 
> I added some comments to code only used for GC.class_stats and 
> GC.class_histogram. I did this when trying to figure them out so I could 
> better understand how to implement VM.class_hierarchy. I can take them out if 
> you think they are not appropriate for this commit.
> 
> One other item I like to discuss is whether it is worth adding a class name 
> argument to this DCMD. That would cause just the superclasses and subclasses 
> of the named class to be printed. If you think that is useful, I think it can 
> be added without too much trouble.
> 
> At the moment not much testing has been done other than running the DCMD and 
> looking at the output. I'll do more once it's clear the code has "settled". I 
> would like to know if there are any existing tests for GC.class_stats and 
> GC.class_histogram (there are none in the "test" directory). If so, possibly 
> one could serve as the basis for a new test for VM.class_hierarchy.
> 
> thanks,
> 
> Chris

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Stefan Karlsson]

Hi Chris,

On 2015-01-08 00:29, Chris Plummer wrote:

Hi,

Please review the following changes for the addition of the 
VM.class_hierarchy DCMD. Please read the bug first for some background 
information.


Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054888


This looks like a nice feature. I think your suggestion about supporting 
a class name argument as the root of the hierarchy would be a nice addition.



Some comments:

Why do you need / use the super_stack? To me it seems like you could 
simplify the could if you get rid of the super_stack and walk the 
Klass::super() chain instead.


Why did you add this side-effect to KlassInfoHisto::print_class_state?:

-  super_index = super_e->index();
+  e->set_super_index(super_e->index());


AFAICT, you are not using KlassInfoHisto::print_class_stats to implement 
the VM.class_hierarchy DCMD, right? Or am I missing something in your patch?


A side-note, if it were not for the AnonymousClasses (created by 
Unsafe_DefineAnonymousClass), then this could have be implemented with 
less resources by just walking the Klass::subclass() and 
Klass::next_sibling() links. Which means that you wouldn't have to use 
any of the classes or functionality in heapInspection.hpp/cpp. But the 
anonymous classes is unfortunately not present in the 
subclass/next_sibling hierarchy.



And some style comments:

http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/services/diagnosticCommand.cpp.frames.html

Maybe it would be nice to move:

  66 #if INCLUDE_SERVICES
  67   DCmdFactory::register_DCmdFactory(new 
DCmdFactoryImpl(full_export, true, false));
  68 #endif

into the already existing INCLUDE_SERVICE block:

  55 #if INCLUDE_SERVICES // Heap dumping/inspection supported
  56   DCmdFactory::register_DCmdFactory(new 
DCmdFactoryImpl(DCmd_Source_Internal | DCmd_Source_AttachAPI, 
true, false));
  57   DCmdFactory::register_DCmdFactory(new 
DCmdFactoryImpl(full_export, true, false));
  58   DCmdFactory::register_DCmdFactory(new 
DCmdFactoryImpl(full_export, true, false));
  59 #endif // INCLUDE_SERVICES


http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.hpp.frames.html


I would prefer if you moved the following implementation to the cpp 
file, so that we can keep our includes in our hpp files to a minimal. 
That helps lowering our include complexity.


 218 inline void KlassInfoEntry::add_subclass(KlassInfoEntry* cie) {
 219   if (_subclasses == NULL) {
 220 _subclasses = new  (ResourceObj::C_HEAP, mtInternal) 
GrowableArray(4, true);
 221   }
 222   _subclasses->append(cie);
 223 }
 224
 225 inline KlassInfoEntry::~KlassInfoEntry() {
 226   if (_subclasses != NULL) {
 227 delete _subclasses;
 228   }
 229 }


http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.cpp.frames.html

Could you move the local variables to where they are used:

 316 void KlassHierarchy::print_class_hierarchy(outputStream* st) {
 317   ResourceMark rm;
 318   int i;
 319   Stack  class_stack;
 320   Stack  super_stack;
 321   GrowableArray elements;
 322


For example, 'int i' into the for statements:

 336   // Set the index for each class
 337   for(i=0; i < elements.length(); i++) {
 338 elements.at(i)->set_index(i+1);
 339   }


Could you add spaces around the operators (= and +):

 337   for(i=0; i < elements.length(); i++) {
 338 elements.at(i)->set_index(i+1);


Some of your new comments are not capitalized and some lack a period. 
Example:


 399   // print indentation with proper indicators of superclass.

 454 // Add the stats for this class to the overall totals


Thanks,
StefanK



I expect there will be further restructuring or additional feature 
work. More discussion on that below. I'm not sure if that additional 
work will be done later with a separately bug filed or with this 
initial commit. That's one thing I want to work out with this review.


Currently the bulk of the DCMD is implemented in heapInspection.cpp. 
The  main purpose of this file is to implement the GC.class_stats and 
GC.class_histogram DCMDs. Both of them require walking the java heap 
to count live objects of each type, thus the name 
"heapInspection.cpp". This new VM.class_hierarchy DCMD does not 
require walking the heap, but is implemented in this file because it 
leverages the existing KlassInfoTable and related classes 
(KlassInfoEntry, KlassInfoBucket, and KlassClosure).


KlassInfoTable makes it easy to build a database of all loaded 
classes, save additional info gathered for each class, iterate over 
them quickly, and also do quick lookups. This exactly what I needed 
for this DCMD, thus the reuse. There is some downside to this. For 
starters, heapInspection.cpp is not the proper place for a DCMD that 
has nothing to do with heap inspection. Also, KlassInfoEntry is being 
overloaded now to suppor

Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

[Mikael Auno]

On 2015-01-08 00:29, Chris Plummer wrote:
> At the moment not much testing has been done other than running the DCMD
> and looking at the output. I'll do more once it's clear the code has
> "settled". I would like to know if there are any existing tests for
> GC.class_stats and GC.class_histogram (there are none in the "test"
> directory). If so, possibly one could serve as the basis for a new test
> for VM.class_hierarchy.

Unfortunately, there are no open tests either of them as of yet. There
are, however, internal ones. I can give you the details offline.

Mikael