[9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
Stefan Karlsson
stefan.karlsson at oracle.com
Fri Jan 9 09:01:02 UTC 2015
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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150109/81119976/attachment-0001.html>
More information about the serviceability-dev
mailing list