RFR (M) JDK-8202605: Standardize on ClassLoaderData::loader_name() throughout the VM to obtain a class loader's name
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Jun 14 20:15:04 UTC 2018
Hi Lois,
On Thu, Jun 14, 2018 at 9:54 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
> 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.
>
Okay, I understand.
>
>>
>> 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!
Sure. It is difficult to unify naming without bothering anyone :)
> 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.
>
Had a quick peek at your webrev, thank you for keeping both _name and
_name_and_id. Also, very nice commenting in classloaderData.cpp. Will
look again tomorrow with a fresh head.
Thanks, Thomas
> 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
>>>
>
More information about the hotspot-dev
mailing list