RFR: 8044107 Add Diagnostic Command to list all ClassLoaders

Tue Jun 3 18:31:26 UTC 2014

On 3 jun 2014, at 15:11, Staffan Larsen <staffan.larsen at oracle.com> wrote:

> Stefan, thanks for the comments. See below.
> 
> On 3 jun 2014, at 11:22, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
>> Hi Staffan,
>> 
>> Thanks for implementing this feature!
>> 
>> On 2014-05-28 16:50, Staffan Larsen wrote:
>>> This change adds a new Diagnostic Command to list all ClassLoaders and some statistics for each classloader. The command is called “GC.classloader_stats” and the information listed is:
>>> 
>>> * An id for the ClassLoader. This is the pointer to the Klass of the ClassLoader. The reason for using the Klass* (instead of the oop) is that it is stable across invocations.
>> 
>> I agree that it's good to get a stable value, but IMHO the code would be easier if you just stored the oops for _classloader and _parent instead of the Klass*.
>> 
>> 71   ClassLoaderData*  _cld;
>> 72   Klass*            _classloader;
>> 73   Klass*            _parent;
>> 
>> 
>> This will work since all this is done during a safe point.
> 
> The reason I choose Klass* was that I wanted something that was stable /across/ invocations, not just during one invocation of the Diagnostic Command. That way you can compare the output over time.

I spoke to Stefan offline and understood that he was ok with printing out the Klass*, but did not like to have them stored in the ClassLoaderStats fields. I misunderstood that.

So here is an updated version that stores class loader and parent as oops, but prints them as Klass*.

http://cr.openjdk.java.net/~sla/8044107/webrev.04/

Thanks,
/Staffan

> 
>> As the code is written right now, it's awkward that we put Klass*s in fields named _parent and _classloader. We usually put oops and CLD*s in fields that are named that way.
> 
> I can change the names of the fields. Suggestions?
> 
>> 
>> The following code:
>> 
>> 144       // It does not exist in our table - add it
>> 145       ClassLoaderStats* cls = new ClassLoaderStats();
>> 146       cls->_classloader = cl->klass();
>> 147       oop parent = java_lang_ClassLoader::parent(cl);
>> 148       if (parent != NULL) {
>> 149         cls->_parent = parent->klass();
>> 150       }
>> 
>> could be condensed down to:
>> 
>> 144       // It does not exist in our table - add it
>> 145       ClassLoaderStats* cls = new ClassLoaderStats();
>> 146       cls->_classloader = cl
>> 147       cls->_parent = java_lang_ClassLoader::parent(cl);
>> 
>> or even:
>> 
>> 144       // It does not exist in our table - add it
>> 145       ClassLoaderStats* cls = new ClassLoaderStats(cl, java_lang_ClassLoader::parent(cl));
>> 
>> 
>> and
>> 
>> 63   cls->_classloader = (cl == NULL ? NULL : cl->klass());
>> 64   if (cl != NULL) {
>> 65     oop parent = java_lang_ClassLoader::parent(cl);
>> 66     if (parent != NULL) {
>> 67       cls->_parent = parent->klass();
>> 68       addEmptyParents(parent);
>> 69     }
>> 70   }
>> 
>> would be come:
>> 
>> 63   cls->_classloader = cl;
>> 64   if (cl != NULL) {
>> 65     cls->_parent = java_lang_ClassLoader::parent(cl);
>> 68     addEmptyParents(cls->_parent);
>> 70   }
>> 
>> 
>>> * The id of the ClassLoader’s parent ClassLoader.
>>> * The pointer to the ClassLoaderData structure in the VM. This can be useful for debugging.
>>> * The number of classes loaded by the ClassLoader.
>>> * The total size of all allocated metaspace chunks for the ClassLoader.
>>> * The total size of all allocated metaspace blocks for the ClassLoader.
>>> 
>>> If there are anonymous classes (invokedynamic classes) attributed to the ClassLoader, the following additional information is listed:
>>> * The number of anonymous classes loaded by the ClassLoader.
>>> * The total size of all allocated metaspace chunks for anonymous classes in the ClassLoader.
>>> * The total size of all allocated metaspace blocks for anonymous classes in the ClassLoader.
>>> 
>>> The information is gathered during a safe point to guarantee that the data structures are consistent.
>>> 
>>> I have added a small test and have run this through jprt. A CCC request has been filed.
>>> 
>>> webrev: http://cr.openjdk.java.net/~sla/8044107/webrev.00/
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8044107
>>> 
>>> Example output:
>>> 
>>> ClassLoader         Parent              CLData*            Classes   ChunkSz   BlockSz  Type
>> 
>> We usually say ClassLoaderData or CLD in the code, but never CLData. Could this be changed to CLD* or CLD? I guess ClassLoaderData would be too long. Erik also had a suggestion on a more user-friendly name, I'm fine with that as well.
> 
> Changed it to CLD*.
> 
>> 
>>> 0x00000007c002d908  0x0000000000000000  0x0000000000000000       0         0         0 sun.misc.Launcher$ExtClassLoader
>>> 0x0000000000000000  0x0000000000000000  0x00007fb239c08de0     761   4694016   4241312  <boot classloader>
>>>                                                                37     75776     50928   + invokedynamic classes
>>> 0x00000007c0061028  0x00000007c0036878  0x00007fb239c2de60       1      6144      1976 ClassLoaderStatsTest$DummyClassLoader
>>>                                                                 1      2048      1288   + invokedynamic classes
>>> 0x00000007c0036878  0x00000007c002d908  0x00007fb239e10fc0       8     88064     31544 sun.misc.Launcher$AppClassLoader
>>> Total = 4                                                      808   4866048   4327048
>>> ChunkSz: Total size of all allocated metaspace chunks
>>> BlockSz: Total size of all allocated metaspace blocks (each chunk has several blocks)
>> 
>> http://cr.openjdk.java.net/~sla/8044107/webrev.02/src/share/vm/classfile/classLoaderStats.hpp.html
>> 
>> Is there a reason why these are intx instead of uintx?
>> 
>> 113   intx    _total_loaders;
>> 114   intx    _total_classes;
> 
> Changed to uintx.
> 
>> 
>> 
>> Would you mind changing the name of _classloader to _class_loader?
>> 
>> 69 class ClassLoaderStats : public ResourceObj {
>> 70 public:
>> 71   ClassLoaderData*  _cld;
>> 72   Klass*            _classloader;
> 
> Fixed.
> 
>> 
>> 
>> 
>> http://cr.openjdk.java.net/~sla/8044107/webrev.02/src/share/vm/classfile/classLoaderStats.cpp.html
>> 
>> There's a couple of usages of SSIZE_FORMAT_W where I think you should be using SIZE_FORMAT_W
> 
> Fixed.
> 
>> 
>> 
>> At some point it would be nice to have an option to be able to list the usages of all individual anonymous class loaders.
>> 
>> 
>> http://cr.openjdk.java.net/~sla/8044107/webrev.02/test/serviceability/dcmd/ClassLoaderStatsTest.java.html
>> 
>> I don't know if this is a common style in our testing, but I often read test code without the aid of an IDE so import statements with *s make it harder for me to understand where imported classes come from:
>> 
>> 31 import java.nio.*;
>> 32 import java.nio.channels.*;
>> 33 import java.io.*;
>> 34 import java.util.regex.Matcher;
>> 35 import java.util.regex.Pattern;
> 
> I agree that this is sloppy. Fixed.
> 
> updated webrev is here: http://cr.openjdk.java.net/~sla/8044107/webrev.03/
> 
> Thanks,
> /Staffan
> 
>> 
>> 
>> thanks,
>> StefanK
>> 
>>> 
>>> 
>>> Thanks,
>>> /Staffan