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

[Lindenmaier, Goetz]

Hi Thomas, 

the change looks good. I don't care about "<unnamed>" etc :)
But I think the "for" in this string is superfluous:
// Print "CLD for [<loader name>,] instance of <loader class name>"
Isn’t it only needed in the anonymous case?
No new webrev needed for removing the "for".

Also I would quote the name, else it might seem to be a word in the 
sentence instead of a name.  Imagine you name the classloader 'test', it reads:
'CLD for test, instance of java.lang.ClassLoader'
I would prefer
CLD "test", instance of java.lang.ClassLoader.
But it might be obvious as you print lists of class loaders, 
so do as you feel in this point.

Best regards,
  Goetz.


> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Thomas Stüfe
> Sent: Samstag, 26. Mai 2018 08:19
> To: Lois Foltan <lois.foltan at oracle.com>
> Cc: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
> Subject: Re: RFR(xs): 8203455: jcmd: VM.metaspace: print loader name for
> anonymous CLDs.
> 
> 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/printCLDMe
> taspaceInfoClosure.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
> >>>
> >>>
> >