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