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

Tue Jun 19 11:28:48 UTC 2018

On 6/15/2018 3:26 PM, Lindenmaier, Goetz wrote:

> 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.
>
Hi Goetz,
Just an update that in the following updated webrev 
http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605.3/, I removed the 
changes in classfile/classLoaderHierarchyDCmd.cpp and 
classfile/classLoaderStats.cpp.  I will make sure your comments above 
make it into the RFE for the serviceability team to look at using 
name_and_id for jcmd.

> The rest is fine. Consider it Reviewed from my side.
>
Good, thanks for the review!
Lois

> 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/
>         <http://cr.openjdk.java.net/%7Elfoltan/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/
>         <http://cr.openjdk.java.net/%7Elfoltan/bug_jdk8202605/webrev/>
>
>             bug link athttps://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
>