RFR (M) JDK-8202605: Standardize on ClassLoaderData::loader_name() throughout the VM to obtain a class loader's name

[Lindenmaier, Goetz]

Hi, 

thanks for this update and for incorporating all my comments!

Looks good, just two comments:

Is it correct to include the ' ' in BOOTSTRAP_LOADER_NAME?
_name does not include ' ' either.
if you do  print("'%s'", loader_name()) you will get 'app' but ''bootstrap''.

In loader_name_and_id you can do 
   return "'" BOOTSTRAP_LOADER_NAME "'";
similar in the jfr file.

But I'm also fine with removing loader_name(), then you only have
cases that need the ' ' around bootstrap :)
I didn't see a use of loader_name() any more, and one can always
call java_lang_ClassLoader::name() (except for during unloading.)

I don't mind the @id printouts in the class loader tree. But is the comment
correct? Doesn't it print the class name twice?

-//      +-- jdk.internal.reflect.DelegatingClassLoader
+//      +-- jdk.internal.reflect.DelegatingClassLoader @<id> jdk.internal.reflect.DelegatingClassLoader

Maybe you need 
ClassLoaderData::loader_name_and_id_prints_classname() { return (strchr(_name_and_id, '\'') == NULL); }
to guard against printing this twice.

Best regards,
  Goetz.




> -----Original Message-----
> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On
> Behalf Of Lois Foltan
> Sent: Donnerstag, 14. Juni 2018 21:56
> To: hotspot-dev developers <hotspot-dev at openjdk.java.net>
> Subject: Re: RFR (M) JDK-8202605: Standardize on
> ClassLoaderData::loader_name() throughout the VM to obtain a class
> loader's name
> 
> Please review this updated webrev that address review comments received.
> 
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605.1/webrev/
> 
> Thanks,
> Lois
> 
> On 6/13/2018 6:58 PM, Lois Foltan wrote:
> > Please review this change to standardize on how to obtain a class
> > loader's name within the VM.  SystemDictionary::loader_name() methods
> > have been removed in favor of ClassLoaderData::loader_name().
> >
> > Since the loader name is largely used in the VM for display purposes
> > (error messages, logging, jcmd, JFR) this change also adopts a new
> > format to append to a class loader's name its identityHashCode and if
> > the loader has not been explicitly named it's qualified class name is
> > used instead.
> >
> > 391 /**
> > 392 * If the defining loader has a name explicitly set then
> > 393 * '<loader-name>' @<id>
> > 394 * If the defining loader has no name then
> > 395 * <qualified-class-name> @<id>
> > 396 * If it's built-in loader then omit `@<id>` as there is only one
> > instance.
> > 397 */
> >
> > The names for the builtin loaders are 'bootstrap', 'app' and 'platform'.
> >
> > open webrev at
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605/webrev/
> > bug link at https://bugs.openjdk.java.net/browse/JDK-8202605
> >
> > Testing: hs-tier(1-2), jdk-tier(1-2) complete
> >                hs-tier(3-5), jdk-tier(3) in progress
> >
> > Thanks,
> > Lois
> >