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.
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?
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.
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
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 }
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
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);
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
Thanks,
StefanK
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
