RFR(xs): 8203455: jcmd: VM.metaspace: print loader name for anonymous CLDs.

[Lois Foltan]

On 5/24/2018 11:01 PM, David Holmes wrote:

> Hi Lois,
>
> On 25/05/2018 6:31 AM, Lois Foltan wrote:
>> On 5/21/2018 3:42 AM, Thomas Stüfe wrote:
>>
>>> Hi all,
>>>
>>> second attempt, after discussing things with David:
>>>
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203455-VM.metaspace-display-loader-name-for-anonymous-cld/webrev.01/webrev/ 
>>>
>>>
>>> The patch is much simpler now. I use
>>> ClassLoaderData::class_loader_name() and ::class_loader_class() which
>>> should always work and be in accordance with planned work in
>>> https://bugs.openjdk.java.net/browse/JDK-8202605.
>>
>> Hi Thomas,
>>
>> Looks good, thanks for using ClassLoaderData::class_loader_name()! A 
>> couple of review comments:
>>
>> - src/hotspot/share/memory/metaspace/printCLDMetaspaceInfoClosure.cpp
>> line #82 - I do not prefer the use of "<unnamed>", but am more in 
>> favor of leaving it out and having the message simply read "CLD for 
>> instance of <loader class name>".  Please consider.
>
> I thought this style had already been put in place:
>
> open/src/hotspot > grepsrc.sh "<unnamed>"
> ./share/classfile/javaClasses.cpp:    name = "<unnamed>";
> ./share/classfile/javaClasses.cpp:        parentName = "<unnamed>";
> ./share/classfile/javaClasses.hpp:  // If a classloader has no name, 
> it prints <unnamed> instead. The output
>
> ??

Hi David,

After discussing this with Karen on Tuesday, going forward, if the 
ClassLoader's name has not been set, we would prefer to leave out 
"<unnamed>" in favor of simply "instance of MyClassLoader" for example.  
Hopefully we can renegotiate this change as well for 
java_lang_ClassLoader::describe_external().

Thanks,
Lois

>
>> line #95 - would prefer <anonymous class> instead of just <anonymous>. 
>
> "VM anonymous class"? (As opposed to Java language anonymous class.)
>
> Thanks,
> David
>
>> According to the comment at line #68, I think you intended that anyways.
>>
>> Thanks,
>> Lois
>>
>>>
>>> Thanks, Thomas
>>>
>>>
>>> On Sun, May 20, 2018 at 8:49 AM, Thomas Stüfe 
>>> <thomas.stuefe at gmail.com> wrote:
>>>> Hi all,
>>>>
>>>> may I please have reviews for this small addition.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203455
>>>> Webrev: 
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203455-VM.metaspace-display-loader-name-for-anonymous-cld/webrev.00/webrev/ 
>>>>
>>>>
>>>> VM.metaspace show-loaders can be used to display loaders (well, really
>>>> CLD instances).
>>>>
>>>> For anonymous CLDs, only "anonymous" is shown. It would be helpful to
>>>> show to which loader these CLD are assigned.
>>>>
>>>> Before:
>>>>
>>>>   "272: ClassLoaderData 0x00007f5ba0538f10 for anonymous class"
>>>>
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203455-VM.metaspace-display-loader-name-for-anonymous-cld/example-before.txt 
>>>>
>>>>
>>>> After patch:
>>>>
>>>>   "268: CLD 0x00007ff0c45738f0 for <anonymous class>, loaded by app,
>>>> instance of jdk.internal.loader.ClassLoaders$AppClassLoader"
>>>>
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203455-VM.metaspace-display-loader-name-for-anonymous-cld/example-after.txt 
>>>>
>>>>
>>>> -- 
>>>> Notes: this patch has a bit more lines than I liked because I did not
>>>> find a singly utility function in ClassloaderData which fit my
>>>> purpose. I wanted to see name and class of the associated loader for
>>>> both normal and unloading case. ClassloaderData::loader_name() has a
>>>> number of shortcomings, I opened
>>>> https://bugs.openjdk.java.net/browse/JDK-8203456 as a follow up rfe.
>>>>
>>>> Thank you,
>>>>
>>>> Thomas
>>