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

Wed Feb 11 08:04:53 UTC 2015

This version looks good to me!

Small comments inline.

> On 11 feb 2015, at 04:13, Chris Plummer <chris.plummer at 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 Plummer<chris.plummer at 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
>>> 
> 
> 