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

[coleen.phillimore at oracle.com]

On 10/5/17 1:38 PM, Jiangli Zhou wrote:
> Hi Calvin,
>
>> On Oct 4, 2017, at 3:32 PM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
>>
>>
>>
>> On 10/4/17, 10:45 AM, mandy chung wrote:
>>>
>>> On 10/4/17 10: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/
>>> systemDictionary.cpp
>>>
>>> SystemDictionary::roots_oops_do and oops_do have to be updated to traverse during GC.
>> It is being handled in the following line of code in roots_oops_do():
>>   CDS_ONLY(SystemDictionaryShared::roots_oops_do(strong);)
> Since you moved _java_platform_loader to systemDictionary.hpp, please handle that in SystemDictionary::roots_oops_do() and SystemDictionary::oops_do().

Yes, agree with Jiangli.  This was a good find, Mandy.

http://cr.openjdk.java.net/~ccheung/8185694/webrev.02/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html

*+ Klass* class_loader_klass = WK_KLASS(ClassLoader_klass);*

This should be:

     InstanceKlass* class_loader_klass = 
SystemDictionary::ClassLoader_klass();

The WK X macro is inappropriate to use here.   I'm surprised it's not 
#undef at this point.

> I agree with David. The checks for 
> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() 
> and 
> SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass() 
> look strange. They should be investigated and cleaned up.
> I've tried simplifying the is_system_class_loader() as mentioned above 
> and it seems to work but more testing is required.

This would be nice if this could be simplified as suggested.

So if you use -Djava.system.loader=myLoader on the command line, setting 
_java_system_loader, then does that mean that the classes loaded by 
SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() 
are not in the system loader?  ie. they can be unloaded?  What is the 
result of the call to SystemDictionary::is_system_class_loader() used 
for?   I guess same question applies to the platform class loader.

thanks,
Coleen

>
>> The implementation is in closed source.
> Please clean up the closed code to remove those.
>
>
> Thanks,
>
> Jiangli
>
>>> Is this new java_platform_loader function used anywhere?
>> Yes, it is being used in closed source.
>>> Currently SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass is preloaded.  Shouldn't this be removed?  What about jdk_internal_loader_ClassLoaders_AppClassLoader?
>> They're being used in lines 184 and 193 in systemDictionary.cpp and also in closed source.
>>> thread.cpp
>>>
>>> 3752 SystemDictionary::compute_java_loaders(CHECK_(JNI_ERR));
>>>
>>> What is the difference between CHECK_(JNI_ERR) vs CHECK_JNI_ERR?  Should it simply use CHECK_JNI_ERR as in other places?
>> They are the same, in utilities/exceptions.hpp:
>> #define CHECK_JNI_ERR                            CHECK_(JNI_ERR)
>>
>> and it expands to the following:
>> __the_thread__); if ((((ThreadShadow*)__the_thread__)->has_pending_exception())) return (-1); (void)(0
>>
>> I can change it to CHECK_JNI_ERR.
>>
>> thanks,
>> Calvin
>>> Mandy