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:23 AM, Lindenmaier, Goetz wrote:
> Hi Lois,
>
> thanks for doing this change!
Hi Goetz,
Thanks for your review and patience while we worked on a proposal for this!

> I appreciate a clear guidance on the naming of classloaders.
> Adding a new field to ClassLoader is the best way to
> assure this is used widely.
I was remiss to not recognize Mandy Chung's contribution of this change 
to ClassLoader.java.

>
> Some comments on the format:
>
> * I would skip the space between the name and the @.
Yes, Mandy & I did discuss this and have concerns either way (with 
space, without space).  For now I am leaving as is and we can 
renegotiate when reviewing the work for improving error message details.

>
> * If two loaders have the same name, it might be helpful
>    to see the classname. But I guess this will mostly
>    happen if the loaders are of the same class, too.
>    (like loading the same library twice. In this case
>     the id helps).
>
> In detail:
>
> classLoaderData.cpp:
>    Please use external_name():
>    +  _class_loader_name_id = _class_loader_klass->name();
Done.

>
>    I would rename loader_name() to loader_nameAndId().
Yes, I will be sending out a new webrev shortly.  I have added back in 
the original loader_name() and now have a new method 
loader_name_and_id().  This should make the intention much clearer.

>
>
> classLoaderData.hpp:
>    Line 400: eventually adapt the comment to use ' ' instead of <>.
Done.

>
> classLoaderHierarchyDCmd.cpp:
>    I would remove the double quotes from this output.
>    But you can leave this to a follow up I guess.
Done.

>
> classLoaderStats.cpp:
>    Maybe you want to adapt classLoaderStats.cpp:114 to say
>    'bootstrap' instead of <boot class loader>.
Done, thanks for that catch!

>
> javaClasses.cpp:
>    +// Returns the name of this class loader or null if this class loader is not named.
>    "the name" is ambiguous now.  Maybe say "Returns the name _field_ of ..."
>
>    +java_lang_ClassLoader::nameAndId(oop loader)
>    I would add to the comment:
>    // Use ClassLoaderData::loader_name() to obtain this String as a char* for internal use.
Done.

>
>
> jfrTypeSetUtils.hpp:
>    Don't you want to put this field into classLoaderData.hpp?
>    Then you can use it in loader_name(), too.
Done.

>
> ClassLoaderHierarchyTest.java:
>    I think you need to edit the other names, too: "Kevin" --> "'Kevin'" etc.
>    Or, if you followed my above comment, remove the double quotes altogether.
Done, good catch.

Again, thanks for your review!
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 00:59
>> To: hotspot-dev developers <hotspot-dev at openjdk.java.net>
>> Subject: RFR (M) JDK-8202605: Standardize on
>> ClassLoaderData::loader_name() throughout the VM to obtain a class
>> loader's name
>>
>> 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