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

[Lindenmaier, Goetz]

Hi Lois,

---------------- stdout ----------------
+-- 'bootstrap',
      |
      +-- 'platform', jdk.internal.loader.ClassLoaders$PlatformClassLoader {0x<address of oop>}
      |     |
      |     +-- 'app', jdk.internal.loader.ClassLoaders$AppClassLoader {0x<address of oop>}
      |
      +-- jdk.internal.reflect.DelegatingClassLoader @20f4f1e0, jdk.internal.reflect.DelegatingClassLoader {0x<address of oop>}

What I mean is that there is twice “jdk.internal.reflect.DelegatingClassLoader”

The printout of the second string could be guarded by

ClassLoaderData::loader_name_and_id_prints_classname() { return (strchr(_name_and_id, '\'') == NULL); }

Adding this function would fit well into your change.
Adapting the output of the jcmd could be left to a follow up,
I think Thomas has stronger feelings here than I do.

Fixing the comment would be great.

The rest is fine. Consider it Reviewed from my side.

Best regards,
  Goetz.


From: Lois Foltan <lois.foltan at oracle.com>
Sent: Friday, June 15, 2018 7:11 PM
To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 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


On 6/15/2018 5:48 AM, Lindenmaier, Goetz wrote:

Hi,



thanks for this update and for incorporating all my comments!
Hi Goetz,
Thanks for another round of review!







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''.
I have removed the ' ' from BOOTSTRAP_LOADER_NAME, good catch.







In loader_name_and_id you can do

   return "'" BOOTSTRAP_LOADER_NAME "'";

similar in the jfr file.
Done.







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 am going to leave ClassLoaderData::loader_name() as is.  It is has the same method name and behavior that currently exists today.  I also want to discourage future changes that directly call java_lang_ClassLoader::name() since as you point out that is not safe during unloading.  Also removing ClassLoaderData::loader_name() may tempt future changes to introduce a new loader_name() method in some data structure other than ClassLoaderData to obtain the java_lang_ClassLoader::name().  Hopefully by leaving loader_name() in, this will prevent ending up back where we are today with multiple ways one can obtain the loader's name.







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.
I believe you are referring to the comment in test serviceability/dcmd/vm/ClassLoaderHierarchyTest.java?  The actual output looks like this:

 Running DCMD 'VM.classloaders' through 'JMXExecutor'
---------------- stdout ----------------
+-- 'bootstrap',
      |
      +-- 'platform', jdk.internal.loader.ClassLoaders$PlatformClassLoader {0x<address of oop>}
      |     |
      |     +-- 'app', jdk.internal.loader.ClassLoaders$AppClassLoader {0x<address of oop>}
      |
      +-- jdk.internal.reflect.DelegatingClassLoader @20f4f1e0, jdk.internal.reflect.DelegatingClassLoader {0x<address of oop>}
      |
      +-- 'Kevin' @3330b2f5, ClassLoaderHierarchyTest$TestClassLoader {0x<address of oop>}
      |
      +-- ClassLoaderHierarchyTest$TestClassLoader @4d81f205, ClassLoaderHierarchyTest$TestClassLoader {0x<address of oop>}
            |
            +-- 'Bill' @4ea761aa, ClassLoaderHierarchyTest$TestClassLoader {0x<address of oop>}

As Mandy pointed out in her review yesterday it really isn't necessary to print out the address of the class loader oop anymore.  I will be opening up a RFE for the serviceability team to address this.  And I will update the comment in the test itself.  Is this acceptable to you?

Thanks,
Lois







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><mailto: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