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

[David Holmes]

On 5/10/2017 8:22 AM, Calvin Cheung wrote:
> 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?

Yep.

>>
>> ---
>>
>> 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);

Yes - exactly.

>>
>> 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.

Unless you really need to determine that the loader is of an actual 
type, you should only be querying java_system_loader() and 
java_platform_loader().

Thanks,
David
-----

> 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