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