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