RFR: 8044107 Add Diagnostic Command to list all ClassLoaders
Staffan Larsen
staffan.larsen at oracle.com
Wed Jun 4 07:50:00 UTC 2014
On 3 jun 2014, at 21:00, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> On 2014-06-03 20:31, Staffan Larsen wrote:
>>
>> 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 for doing these changes.
>
> I think I found a bug in the test:
> 86 Matcher m1 = anonLine.matcher(next);
> 87 m1.matches();
> 88 if (!m1.group(1).equals("1")) {
> 89 throw new Exception("Should have loaded 1 anonymous class, but found : " + m1.group(1));
> 90 }
> 91 checkPositiveInt(m.group(2));
> 92 checkPositiveInt(m.group(3));
>
> Shouldn't line 91 and 92 be looking at m1?
Yes, they should! I fixed that and the test still passes.
>
> Other than that, this looks good.
Thanks!
/Staffan
>
> thanks,
> StefanK
>>
>> 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
>>
>
More information about the hotspot-dev
mailing list