[9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy

Thu Jan 8 10:50:33 UTC 2015

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