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

[Thomas Stüfe]

Thanks Lois!

Could I have a second reviewer please?

Best Regards, Thomas

On Fri, May 25, 2018 at 8:52 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
> On 5/24/2018 5:25 PM, Thomas Stüfe wrote:
>
>> Thanks for the review Lois!
>>
>> See comments inline.
>>
>> On Thu, May 24, 2018 at 10:31 PM, Lois Foltan <lois.foltan at oracle.com>
>> 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.
>>
>> Sure - the majority of loaders seem not yet to have names, so this
>> would remove a lot of "<unnamed>".
>>
>>> line #95 - would prefer <anonymous class> instead of just <anonymous>.
>>> According to the comment at line #68, I think you intended that anyways.
>>>
>> Fixed.
>>
>>> Thanks,
>>> Lois
>>>
>> New webrev:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8203455-VM.metaspace-display-loader-name-for-anonymous-cld/webrev.02/webrev/src/hotspot/share/memory/metaspace/printCLDMetaspaceInfoClosure.cpp.udiff.html
>
>
> Thomas,
> Looks good, thank you for making those changes!
> Lois
>
>
>>
>> Example output:
>>
>> anonymous class, loader has name:
>>    5: CLD 0x00007f814c47a7d0 for <anonymous class>, loaded by app
>> instance of jdk.internal.loader.ClassLoaders$AppClassLoader
>>
>> anonymous class in <boot>
>>    6: CLD 0x00007f814c474ae0 for <anonymous class>, loaded by <bootstrap>
>>
>> loader has name and class name:
>>    7: CLD 0x00007f814c3afc60 for app instance of
>> jdk.internal.loader.ClassLoaders$AppClassLoader
>>
>> <boot>:
>>    8: CLD 0x00007f814c1d8570 for <bootstrap>
>>
>> Thank you,
>>
>> Thomas
>>
>>>> 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
>>>
>>>
>