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

[Lois Foltan]

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