On 01/08/2015 10:29 PM, Chris Plummer wrote:
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.
It seems to me that we have two very different use cases here, each one
best served with a different output format:
1 - Listing of all classes/interfaces hierarchy when the dcmd is
invoked without arguments:
-> Chris' output format as described below (with interfaces)
2 - Investigation on a particular class or interface when a class
or interface is passed in argument to the dcmd
-> Karen's output format, much easier to work with to
track default methods. Because the output is limited to the
hierarchy from a single class, there's no class duplication
in output (single parent class inheritance) and limited
interfaces duplication.
If the implementations of the two features are too different, we could
consider having two different dcmds.
My 2 cents,
Fred
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
--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com
