RFR (M) JDK-8202605: Standardize on ClassLoaderData::loader_name() throughout the VM to obtain a class loader's name
lois.foltan at oracle.com
Thu Jun 14 19:30:53 UTC 2018
On 6/14/2018 8:23 AM, Lindenmaier, Goetz wrote:
> Hi Lois,
> thanks for doing this change!
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
> 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:
> Please use external_name():
> + _class_loader_name_id = _class_loader_klass->name();
> 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.
> Line 400: eventually adapt the comment to use ' ' instead of <>.
> I would remove the double quotes from this output.
> But you can leave this to a follow up I guess.
> Maybe you want to adapt classLoaderStats.cpp:114 to say
> 'bootstrap' instead of <boot class loader>.
Done, thanks for that catch!
> +// 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.
> Don't you want to put this field into classLoaderData.hpp?
> Then you can use it in loader_name(), too.
> 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!
> Best regards,
>> -----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
>> 397 */
>> The names for the builtin loaders are 'bootstrap', 'app' and 'platform'.
>> open webrev at
>> 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
More information about the hotspot-dev