Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 chris.plum...@oracle.com 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
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
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
Looks good! Thanks for providing incremental webrevs - very helpful! Thanks, /Staffan On 12 feb 2015, at 09:31, Chris Plummer chris.plum...@oracle.com 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.02-03/ http://cr.openjdk.java.net/~cjplummer/8054888/webrev.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 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
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
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
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 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 chris.plum...@oracle.com 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 Plummerchris.plum...@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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
This version looks good to me!
Small comments inline.
On 11 feb 2015, at 04:13, Chris Plummer chris.plum...@oracle.com 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 Plummerchris.plum...@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 Plummerchris.plum...@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 chris.plum...@oracle.com 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
On 6 feb 2015, at 21:15, Chris Plummer chris.plum...@oracle.com 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=classloaderID 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 chris.plum...@oracle.com mailto:chris.plum...@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
On 6 feb 2015, at 22:21, Karen Kinnear karen.kinn...@oracle.com 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 boot class loader 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.Serializable/0x0007c00375f8 (java.base, intf) |--java.util.RandomAccess/0x0007c00375f8 (java.base, intf) ...
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 did this when trying to figure them out so
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
[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
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 ioi@oracle.com 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 ioi@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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=classloaderID 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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=classloaderID 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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
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 chris.plum...@oracle.com 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
[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
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] [classname] Arguments:
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
[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
On 9 jan 2015, at 21:08, Chris Plummer chris.plum...@oracle.com 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 frederic.par...@oracle.com
mailto:frederic.par...@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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. Love
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 frederic.par...@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
On 9 jan 2015, at 18:49, Karen Kinnear karen.kinn...@oracle.com 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 frederic.par...@oracle.com
mailto:frederic.par...@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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
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
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
DCmdFactoryImplClassHierarchyDCmd(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
DCmdFactoryImplHeapDumpDCmd(DCmd_Source_Internal |
DCmd_Source_AttachAPI, true, false));
57 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassHistogramDCmd(full_export, true, false));
58 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassStatsDCmd(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)
GrowableArrayKlassInfoEntry*(4, true);
221 }
222 _subclasses-append(cie);
223 }
224
225 inline KlassInfoEntry::~KlassInfoEntry() {
226 if (_subclasses != NULL) {
227 delete
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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 frederic.par...@oracle.com 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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
DCmdFactoryImplClassHierarchyDCmd(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
DCmdFactoryImplHeapDumpDCmd(DCmd_Source_Internal | DCmd_Source_AttachAPI,
true, false));
57 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassHistogramDCmd(full_export, true, false));
58 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassStatsDCmd(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)
GrowableArrayKlassInfoEntry*(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 KlassInfoEntry*, mtClass class_stack;
320 Stack KlassInfoEntry*, mtClass super_stack;
321 GrowableArrayKlassInfoEntry* 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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
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
DCmdFactoryImplClassHierarchyDCmd(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
DCmdFactoryImplHeapDumpDCmd(DCmd_Source_Internal |
DCmd_Source_AttachAPI, true, false));
57 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassHistogramDCmd(full_export, true,
false));
58 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassStatsDCmd(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)
GrowableArrayKlassInfoEntry*(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,
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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
[9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
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
