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

[Thomas Stüfe]

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.

---

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>

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.

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
>