On 2015-01-08 20:15, Chris Plummer wrote:
Hi Stefan,
Comments inline below:
On 1/8/15 2:50 AM, Stefan Karlsson wrote:
Hi Chris,
On 2015-01-08 00:29, Chris Plummer wrote:
Hi,
Please review the following changes for the addition of the
VM.class_hierarchy DCMD. Please read the bug first for some
background information.
Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054888
This looks like a nice feature. I think your suggestion about
supporting a class name argument as the root of the hierarchy would
be a nice addition.
Some comments:
Why do you need / use the super_stack? To me it seems like you could
simplify the could if you get rid of the super_stack and walk the
Klass::super() chain instead.
The initial implementation actually printed the superclass index as
the indentation, which required the super_stack, and I just ended up
keeping it. It looks like I could get rid of all the superclass
maintenance done in print_class_hierarchy() if I walk Klass:super()
in print_class(). I'll make that change.
Why did you add this side-effect to KlassInfoHisto::print_class_state?:
- super_index = super_e->index();
+ e->set_super_index(super_e->index());
AFAICT, you are not using KlassInfoHisto::print_class_stats to
implement the VM.class_hierarchy DCMD, right? Or am I missing
something in your patch?
Originally I did overload print_class_stats to also handle
VM.class_hierarchy, but in the end decided not to due to the overhead
of it causing the java heap to be walked. So the above is a remnant,
but is also consistent with how print_class_hierarchy() sets the
super_index field of the KlassInfoEntry. However, it may not be
necessary anymore to maintain the super_index if I make the change
above to no longer maintain the super_stack. I'll look into that.
A side-note, if it were not for the AnonymousClasses (created by
Unsafe_DefineAnonymousClass), then this could have be implemented
with less resources by just walking the Klass::subclass() and
Klass::next_sibling() links. Which means that you wouldn't have to
use any of the classes or functionality in heapInspection.hpp/cpp.
But the anonymous classes is unfortunately not present in the
subclass/next_sibling hierarchy.
I didn't not realize subclass info was being maintained by Klass.
Still had CVM stuck in my head, which does not do that. I'm not sure
what the AnonymousClasses issue is. Can you explain more?
The AnonymousClasses are supposed to be lightweight classes used by
JSR292. They have a different lifecycle than ordinary Klasses, and are
not registered in some of the data structures where the ordinary Klasses
are registered, and one example is the subclass/next_sibling tree.
Because these classes treated differently, they are a constant source of
bugs. If you visit Klasses by iterating over a data structure in the
JVM, you need to know if the AnonymousClasses are present our not. Your
current code is safe because you walk the ClassLoaderDataGraph, which
contains the AnonymousKlasses. But a simplified version relying on the
subclass/next_sibling tree would unfortunately be broken. The
SystemDictionary is another place where the AnonymousClasses are not
registered.
Note, that the term "anonymous class" is an overloaded term and I'm not
referring to this kind of anonymous classes:
http://docs.oracle.com/javase/tutorial/java/javaOO/anonymousclasses.html
And some style comments:
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/services/diagnosticCommand.cpp.frames.html
Maybe it would be nice to move:
66 #if INCLUDE_SERVICES
67 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImpl<ClassHierarchyDCmd>(full_export, true, false));
68 #endif
into the already existing INCLUDE_SERVICE block:
55 #if INCLUDE_SERVICES // Heap dumping/inspection supported
56 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImpl<HeapDumpDCmd>(DCmd_Source_Internal |
DCmd_Source_AttachAPI, true, false));
57 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImpl<ClassHistogramDCmd>(full_export, true, false));
58 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImpl<ClassStatsDCmd>(full_export, true, false));
59 #endif // INCLUDE_SERVICES
Ok.
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.hpp.frames.html
I would prefer if you moved the following implementation to the cpp
file, so that we can keep our includes in our hpp files to a minimal.
That helps lowering our include complexity.
218 inline void KlassInfoEntry::add_subclass(KlassInfoEntry* cie) {
219 if (_subclasses == NULL) {
220 _subclasses = new (ResourceObj::C_HEAP, mtInternal)
GrowableArray<KlassInfoEntry*>(4, true);
221 }
222 _subclasses->append(cie);
223 }
224
225 inline KlassInfoEntry::~KlassInfoEntry() {
226 if (_subclasses != NULL) {
227 delete _subclasses;
228 }
229 }
I guess I'm a bit unclear on this, and had already pondered where this
code should go. I've seen methods embedded in the class definition,
inline in a separate "xxx_inline.hpp" file, inline like I have above,
and of course in the .cpp file. What are the guidelines for deciding
where to put them?
There's a section written on:
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#Files
But typically, don't add the implementation in the hpp if the code
depends on other hpp files. If the the code needs to be inlined for
performance reasons, then you should put the declaration in a
xxx.inline.hpp file.
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.cpp.frames.html
Could you move the local variables to where they are used:
316 void KlassHierarchy::print_class_hierarchy(outputStream* st) {
317 ResourceMark rm;
318 int i;
319 Stack <KlassInfoEntry*, mtClass> class_stack;
320 Stack <KlassInfoEntry*, mtClass> super_stack;
321 GrowableArray<KlassInfoEntry*> elements;
322
ok.
For example, 'int i' into the for statements:
336 // Set the index for each class
337 for(i=0; i < elements.length(); i++) {
338 elements.at(i)->set_index(i+1);
339 }
Could you add spaces around the operators (= and +):
337 for(i=0; i < elements.length(); i++) {
338 elements.at(i)->set_index(i+1);
ok.
Some of your new comments are not capitalized and some lack a period.
Example:
399 // print indentation with proper indicators of superclass.
454 // Add the stats for this class to the overall totals
Ok.
Thanks,
StefanK
Thanks for the review!
Thanks for the feature!
StefanK
Chris
I expect there will be further restructuring or additional feature
work. More discussion on that below. I'm not sure if that additional
work will be done later with a separately bug filed or with this
initial commit. That's one thing I want to work out with this review.
Currently the bulk of the DCMD is implemented in heapInspection.cpp.
The main purpose of this file is to implement the GC.class_stats
and GC.class_histogram DCMDs. Both of them require walking the java
heap to count live objects of each type, thus the name
"heapInspection.cpp". This new VM.class_hierarchy DCMD does not
require walking the heap, but is implemented in this file because it
leverages the existing KlassInfoTable and related classes
(KlassInfoEntry, KlassInfoBucket, and KlassClosure).
KlassInfoTable makes it easy to build a database of all loaded
classes, save additional info gathered for each class, iterate over
them quickly, and also do quick lookups. This exactly what I needed
for this DCMD, thus the reuse. There is some downside to this. For
starters, heapInspection.cpp is not the proper place for a DCMD that
has nothing to do with heap inspection. Also, KlassInfoEntry is
being overloaded now to support 3 different DCMDs, as is
KlassInfoTable. As a result each has a few fields and methods that
are not used for all 3 DCMDs. Some subclassing might be in order
here, but I'm not sure if it's worth it. Opinions welcomed. If I am
going to refactor, I would prefer that be done as a next step so I'm
not disturbing the existing DCMDs with this first implementation.
I added some comments to code only used for GC.class_stats and
GC.class_histogram. I did this when trying to figure them out so I
could better understand how to implement VM.class_hierarchy. I can
take them out if you think they are not appropriate for this commit.
One other item I like to discuss is whether it is worth adding a
class name argument to this DCMD. That would cause just the
superclasses and subclasses of the named class to be printed. If you
think that is useful, I think it can be added without too much trouble.
At the moment not much testing has been done other than running the
DCMD and looking at the output. I'll do more once it's clear the
code has "settled". I would like to know if there are any existing
tests for GC.class_stats and GC.class_histogram (there are none in
the "test" directory). If so, possibly one could serve as the basis
for a new test for VM.class_hierarchy.
thanks,
Chris
