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 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 >>> > >
