RFR(S): 8185694: Replace SystemDictionaryShared::_java_platform_loader with SystemDictionary::is_platform_class_loader()

Calvin Cheung calvin.cheung at oracle.com
Wed Oct 4 22:22:42 UTC 2017



On 10/4/17, 2:41 PM, David Holmes wrote:
> Hi Calvin,
>
> On 5/10/2017 3:12 AM, Calvin Cheung wrote:
>> Mandy, David,
>>
>> Thanks for the review.
>>
>> I've made the change based on your suggestion:
>> http://cr.openjdk.java.net/~ccheung/8185694/webrev.01/
>
> share/classfile/systemDictionary.cpp
>
> There's an existing bug that this local is not used:
>
> 132   Klass* system_klass = WK_KLASS(ClassLoader_klass);
>
> (and the variable is poorly named). Now that you need to use 
> WK_KLASS(ClassLoader_klass) twice you should instead use the local.
How about changing the local variable name to class_loader_klass and 
using it at lines 135 and 143?
>
> ---
>
> As Mandy alludes these:
>
> 177 // Returns true if the passed class loader is the builtin 
> application class loader
>  178 // or a custom system class loader. A customer system class 
> loader can be
>  179 // specified via -Djava.system.class.loader.
>  180 bool SystemDictionary::is_system_class_loader(oop class_loader) {
>  181   if (class_loader == NULL) {
>  182     return false;
>  183   }
>  184   return (class_loader->klass() == 
> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() 
> ||
>  185           class_loader == _java_system_loader);
>  186 }
>  187
>  188 // Returns true if the passed class loader is the platform class 
> loader.
>  189 bool SystemDictionary::is_platform_class_loader(oop class_loader) {
>  190   if (class_loader == NULL) {
>  191     return false;
>  192   }
>  193   return (class_loader->klass() == 
> SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass());
>  194 }
>
> would seem to be checking the wrong things - they should just be 
> checking java_system_loader() and java_platform_loader() shouldn't they?
Do you mean lines 184 and 185 could become the following?

     return (class_loader == _java_system_loader);

and line 193 could become the following?

     return (class_loader == _java_platform_loader);
>
> In the case of is_system_class_loader() it seems odd that it checks a 
> specific hard-wired class as well as the defined _java_system_loader ??
Not sure. That code has been there for a long time.

Also, both 
SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() and 
SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass() 
are being used in the closed source for JavaCalls.

thanks,
Calvin
>
> Thanks,
> David
> -----
>
>
>
>> thanks,
>> Calvin
>>
>> On 10/4/17, 8:29 AM, mandy chung wrote:
>>>
>>>
>>> On 10/3/17 11:36 PM, David Holmes wrote:
>>>> Hi Calvin,
>>>>
>>>> On 4/10/2017 7:26 AM, Calvin Cheung wrote:
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8185694
>>>>>
>>>>> The open code change for this fix involves adding the 
>>>>> _java_platform_loader to the SystemDictionary class.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8185694/webrev.00/
>>>>
>>>> I think you should be calling getPlatformClassLoader rather than 
>>>> poking at the parent field of the system loader. That way you don't 
>>>> have to worry about whether or not the system loader has been 
>>>> customized by the user.
>>> Agree.  ClassLoader::getPlatformClassLoader is the right way to get 
>>> the platform class loader.
>>>
>>> Mandy


More information about the hotspot-runtime-dev mailing list