Hi Karen,
Comments inline.
On 1/8/15 8:07 AM, Karen Kinnear wrote:
Chris,
Thank you for doing this. I had a couple of questions/comments.
I like your idea of being able to start with a specific class to show all
subclasses of.
Ok. I'll add that.
I think the way I read the code this shows the superclass hierarchy, not the
superinterfaces. With the addition
of default methods in interfaces, I think we have increased the value in seeing
superinterfaces.
It does include interfaces in the output, but not as part of any class
hierarchy. Interfaces are just shown as regular classes that inherit
from Object. This is the case if one interface extends another, I
suppose because "extends" is just interpreted as "implements" in this case.
So what I personally would find useful would be to be able to start with a
specific class and
find the superclasses and superinterfaces of that class - for the debugging I
do, I usually am
trying to look up and need both sets of information. Today if you run
-XX:+TraceDefaultMethods
there is one sample way to display all the supertypes of a single type, all the
way up. I don't know the
best way to make that consistent with the current output approach, e.g. using
the |- syntax.
e.g.
Class java.util.Arrays$ArrayList requires default method processing
java/util/Arrays$ArrayList
java/util/AbstractList
java/util/AbstractCollection
java/lang/Object
java/util/Collection
java/lang/Object
java/lang/Iterable
java/lang/Object
java/util/List
java/lang/Object
java/util/Collection
java/lang/Object
java/lang/Iterable
java/lang/Object
java/util/RandomAccess
java/lang/Object
java/io/Serializable
java/lang/Object
Do you think there could be value to others in the ability to walk up the
hierarchy as well as to
see superclasses and superinterfaces at least from that perspective?
This is a inverted from how my dcmd prints the hierarchy, plus also
includes interfaces. Inverting the hierarchy means a class is listed
with every subclass of the class, which I don't think is desirable.
Including interfaces has the same issue, but introduces a new issue even
when not inverting the hierarchy. The same interface can be in more than
one location in the hierarchy, so there is no avoiding printing it more
than once, so we need to decide how to best include interfaces in the
output. The could just be a simple list right after the class that
implements them:
java.lang.Object
| ...
|--MyBaseClass
| | implements -> MyInterface1
| | implements -> MyInterface2
| |--MySubClass
| implements -> MyInterface1
| implements -> MyInterface2
| ...
|--MyInterface1
|--MyInterface2
The "implements" lines could be optional.
I know cvm would distinguish between interfaces the Class declared it
implemented, and those it inherited from the interfaces it declared it
implemented. This was necessary for reflection, and I think also to
properly build up interfaces tables. I assume hotspot does something
similar. Is there any need for this information to be conveyed in the
above output, or just list out every interface implemented, and not
worry about how the class acquired it.
Is there value in printing the defining class loader for each class - maybe
optionally?
This is already available with GC.class_stats, although not in the
default output. I suppose the reality is that it might be better handled
by this DCMD. The main puprose of GC.class_stats is to print statistics
(counts and sizes), so printing the ClassLoader name there is probably
not appropriate, but then it's not really appropriate for
VM.class_hierarchy either. I'm not sure how best to handle this. One or
both DCMDs possibly should be re-purposed and more clearly define what
type of data it displays.
If so, I'm wondering if there might be value in future for the jigsaw project
adding the module for each class - maybe optionally as well?
This relates to my above statement. We need to figure out what type of
data is useful, and which dcmds should handle them.
Love opinions on that - especially from the serviceability folks
thanks,
Karen
Thanks for the input.
Chris
On Jan 7, 2015, at 6:29 PM, 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
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
