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

[Chris Plummer]

Hi Staffan,

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

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

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

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

On 2/10/15 4:19 AM, Staffan Larsen wrote:
> Chris,
>
> In general I think this looks very good. Simple and well-commented code to follow. I am missing a test, though. Please look at the hotspot/test/serviceability/dcmd set of tests.
Added.
>
>
> A couple of smaller comments:
>
>
> Are Unsafe.defineAnonymousClass classes included? Should they be?
I didn't really understand what you were talking about at first, but 
then when I started looking at ClassLoaderStatsTest.java, I saw the 
following:

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

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

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

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

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

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

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

thanks,

Chris
>
> Thanks,
> /Staffan
>
>
>
>   
>> On 10 feb 2015, at 03:00, Chris Plummer<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
>>