> On 9 jan 2015, at 21:08, Chris Plummer <chris.plum...@oracle.com> wrote:
>
> On 1/9/15 4:38 AM, Staffan Larsen wrote:
>> It�s getting difficult to get all the information into the same output:
>> hierarchy, interfaces, class loaders and modules. I took a stab at it and it
>> could look like this:
>>
>> java.lang.Object
>> |--java.io.Serializable (java.base, 0x00000007c00375f8, iface)
>> |--java.util.RandomAccess (java.base, 0x00000007c00375f8, iface)
>> |--java.lang.Iterable (java.base, 0x00000007c00375f8, iface)
>> | |--java.util.Collection (java.base, 0x00000007c00375f8, iface)
>> | | |--java.util.List (java.base, 0x00000007c00375f8, iface)
>> |--java.util.AbstractCollection (java.base, 0x00000007c00375f8)
>> | | implements java.util.Collection
>> | | implements java.lang.Iterable
>> | |--java.util.AbstractList (java.base, 0x00000007c00375f8)
>> | | implements java.util.List
>> | | |--java.util.Arrays$ArrayList (java.base, 0x00000007c00375f8)
>> | | | implements java.io.Serializable
>> | | | implements java.util.RandomAccess
> For the most part I like this. The one part I'm not too fond of is hex long
> which I assume represents the ClassLoader. Maybe we could just use a simple
> index, and at the end of the dump include a table of the referenced
> ClassLoaders. Possibly just hijack what VM.classloader_stats outputs, but
> include the simple index in the first column.
>>
>>
>> But this is pushing what can be visualized in one place.
>>
>> We will need to add module and class loader information somehow to all the
>> Diagnostic Commands that list classes. In addition we will need a way to see
>> how modules relate to one another. Perhaps it isn�t possible to have all the
>> information in one place, but instead make it possible to cross reference
>> between different diagnostic commands. For example, GC.class_stats could
>> have the module and class loader information, and GC.class_hierarchy would
>> not have to include it. What is missing from making that possible is a
>> unique way of identifying a class (since the name is not unique). All output
>> would need to include that unique identifier and it would be possible to
>> cross reference. The identifier has to be stable during a JVM run, but not
>> between runs.
> I don't know if there is some sort of simple stable ID for a class other than
> the address of the Klass instance. And of course this assumes there isn't any
> class unloading going on that could result in reuse of the address. Is that
> acceptable?
Yeah, I was thinking about the Klass*, but as you say it isn’t guaranteed to be
unique over time. Karen suggested using {class loader, class} which has the
same problem, but includes more useful information, so I think that is
preferable. (Ideally we should have a unique id assigned to classes and class
loaders when they are created).
>>
>> The above would then become:
>>
>> java.lang.Object/0x12345600
>> |--java.io.Serializable/0x12345601
>> |--java.util.RandomAccess/0x12345602
>> |--java.lang.Iterable/0x12345603
>> | |--java.util.Collection/0x12345604
>> | | |--java.util.List/0x12345605
>> |--java.util.AbstractCollection/0x12345606
>> | | implements java.util.Collection/0x12345604
>> | | implements java.lang.Iterable/0x12345603
>> | |--java.util.AbstractList/0x12345607
>> | | implements java.util.List/0x12345605
>> | | |--java.util.Arrays$ArrayList/0x12345608
>> | | | implements java.io.Serializable/0x12345601
>> | | | implements java.util.RandomAccess/0x12345602
>>
>> With additions to GC.class_stats:
>>
>> Index Super ClassLoader ClassName
>> 1 -1 0x00000007c0034c48 java.lang.Object/0x12345600
>> 2 1 0x00000007c0034c48 java.util.List/0x12345605
>> 3 31 0x00000007c0034c48 java.util.AbstractList/0x12345607
>>
>> And GC.class_histogram:
>>
>> num #instances #bytes class name
>> ----------------------------------------------
>> 1: 945 117736 java.lang.Object/0x12345600
>> 2: 442 50352 java.util.List/0x12345605
>> 3: 499 25288 java.util.AbstractList/0x12345607
> Would it be better for the class identifier to be in a separate column from
> the class name, or is combining them intentional? In any case, it's easy
> enough to add to all of the above assuming we are talking about an existing
> value such as the address of the Klass instance.
Perhaps better to separate them, yes.
/Staffan
>
> Chris
>>
>>
>>
>> /Staffan
>>
>>> On 9 jan 2015, at 09:53, Frederic Parain <frederic.par...@oracle.com
>>> <mailto:frederic.par...@oracle.com>> wrote:
>>>
>>>
>>>
>>> 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/
>>>>>> <http://cr.openjdk.java.net/%7Ecjplummer/8054888/webrev.00/>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8054888
>>>>>> <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 <mailto:frederic.par...@oracle.com>
>
