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

[Karen Kinnear]

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

thanks,
Karen

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

> This version looks good to me!
> 
> Small comments inline.
> 
>> On 11 feb 2015, at 04:13, Chris Plummer <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
>>>> 
>> 
>> 
>