RFR: 8044107 Add Diagnostic Command to list all ClassLoaders
Stefan Karlsson
stefan.karlsson at oracle.com
Tue Jun 3 19:00:11 UTC 2014
On 2014-06-03 20:31, Staffan Larsen wrote:
>
> On 3 jun 2014, at 15:11, Staffan Larsen <staffan.larsen at oracle.com
> <mailto: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
>> <mailto: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/
> <http://cr.openjdk.java.net/%7Esla/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?
Other than that, this looks good.
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/
>>>> <http://cr.openjdk.java.net/%7Esla/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
>>> <http://cr.openjdk.java.net/%7Esla/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
>>> <http://cr.openjdk.java.net/%7Esla/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
>>> <http://cr.openjdk.java.net/%7Esla/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/
>> <http://cr.openjdk.java.net/%7Esla/8044107/webrev.03/>
>>
>> Thanks,
>> /Staffan
>>
>>>
>>>
>>> thanks,
>>> StefanK
>>>
>>>>
>>>>
>>>> Thanks,
>>>> /Staffan
>
More information about the hotspot-dev
mailing list