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