Thanks Frederic for suggesting two different dcmds - they could share a lot of the code logic.
If folks generally prefer these as separate dcmds - I can file an rfe to add the inverted one - i.e. start at a given class/interface and tell me its supertypes. thanks, Karen On Jan 9, 2015, at 3:53 AM, Frederic Parain 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/ >>>> 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