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

[Lois Foltan]

On 6/14/2018 8:51 AM, Thomas Stüfe wrote:

> Hi Lois,
>
> it is a good thing to introduce a common naming scheme plus guidelines
> of how to use them. I like that you introduced this to both
> ClassLoaderData and ClassLoader.java - that way we can have a common
> scheme regardless whether we have a CLD pointer or a ClassLoader oop.

Hi Thomas,
Thanks for your review! As I pointed out to Goetz, Mandy Chung 
contributed this change to ClassLoader.java.

>
> ---
>
> But of course I have complaints too :)
>
> I dislike the compound format:
>
>   - it hides the class name if the loader name is set.
>   - it adds quotes, which may not be desired in all places.
>   - names of the special loaders bootstrap I would prefer to keep
> without quotes but with angular brackets:
>     'bootstrap' -> <bootstrap>, 'app' -> <app>, 'platform' -> <platform>

The names for the builtin loaders ('bootstrap', 'app', 'platform') was 
decided in the JDK 9 time frame.  Any suggestion to change it would have 
to involve the core library, hotspot groups, etc.  It is important that 
the JVM use the names given to these loaders to provide more helpful and 
accurate output.

>
> Since your patch changes how some of my jcmd subcommands print things,
> they now look not so good. For example VM.classloaders:
>
> Before we had:
>
> 938:
> +-- <bootstrap>
>        |
>        +-- jdk.internal.reflect.DelegatingClassLoader {0x000000071523ae00}
>        |
>        |                     Classes:
> jdk.internal.reflect.GeneratedConstructorAccessor1 (invokes:
> java/lang/management/ManagementPermission::<init>
> (Ljava/lang/String;)V)
>        |                              (1 class)
>        |
>        +-- "Kevin", ClassLoaderHierarchyTest$TestClassLoader {0x000000071529f620}
>        |
>        |                     Classes: TestClass2
>        |                              (1 class)
>        |
>        |           Anonymous Classes: TestClass2$$Lambda$46/0x000000080011cc40
>        |                              (1 anonymous class)
>
>
> With your patch:
>
> VM.classloaders
> 13580:
> +-- "'bootstrap'",
>        |
>        |
>        +-- "jdk.internal.reflect.DelegatingClassLoader @41cbc171",
> jdk.internal.reflect.DelegatingClassLoader {0x000000071523a638}
>        |
>        |                     Classes:
> jdk.internal.reflect.GeneratedConstructorAccessor1 (invokes:
> java/lang/management/ManagementPermission::<init>
> (Ljava/lang/String;)V)
>        |                              (1 class)
>        |
>        +-- "'Kevin' @9904154", ClassLoaderHierarchyTest$TestClassLoader
> {0x000000071529f1e8}
>        |
>        |                     Classes: TestClass2
>        |                              (1 class)
>        |
>        |           Anonymous Classes: TestClass2$$Lambda$46/0x000000080011cc40
>        |                              (1 anonymous class)
>
>
> Of course this could be improved a bit - if I were to use your new
> function and tweak it a bit, it would be:
>
> VM.classloaders
> 13580:
> +-- 'bootstrap',
>        |
>        |
>        +-- jdk.internal.reflect.DelegatingClassLoader @41cbc171
> {0x000000071523a638}
>        |
>        |                     Classes:
> jdk.internal.reflect.GeneratedConstructorAccessor1 (invokes:
> java/lang/management/ManagementPermission::<init>
> (Ljava/lang/String;)V)
>        |                              (1 class)
>        |
>        +-- 'Kevin' @9904154 {0x000000071529f1e8}
>        |
>        |                     Classes: TestClass2
>        |                              (1 class)
>        |
>        |           Anonymous Classes: TestClass2$$Lambda$46/0x000000080011cc40
>        |                              (1 anonymous class)
>
> but again, I still loose the class name for loaders which have names,
> or print the class name twice for loaders without name.
>
> ----
>
> So, could we not keep the old ClassLoaderData::class_loader_name()
> unchanged, and add the new compound name with a (clearly named) new
> function, e.g. "ClassLoaderData::class_loader_name_and_id()" or
> "ClassLoaderData::class_loader_compund_name()" or similar?
>
> I appreciate your wish to unify all naming, but I find this overly
> restrictive, see above examples.
>
> Also, I really like methods doing what they are named to do - I
> dislike too-smart methods which force me to second-guess their
> function: ClassLoaderData::class_loader_name() suggests it does just
> that, returning "ClassLoader.name()". Now it returns the new compound
> format. This is not appearant from the naming.

Sometimes one size doesn't fit all!  I will be sending out a new webrev 
shortly that introduces the fields ClassLoaderData::_name and 
ClassLoaderData::_name_and_id with corresponding methods to obtain 
each.  I have backed out my change specifically to 
memory/metaspace/printCLDMetaspaceInfoClosure.cpp & test 
Metaspace/PrintMetaspaceDcmd.java.

Thanks,
Lois

>
> Thanks and Kind Regards,
>
> Thomas
>
>
>
>
>
> On Thu, Jun 14, 2018 at 12:58 AM, Lois Foltan <lois.foltan at oracle.com> 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
>>