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

[Calvin Cheung]

On 10/5/17, 1:54 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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've modified the code to using SystemDictionary::ClassLoader_klass() 
but I don't need the local variable because I'm creating the 
_java_system_loader and _java_platform_loader separately in 2 different 
functions.

It is because the _java_platform_loader is required for module system 
initialization in call_initPhase2.
It would be too early to create the _java_system_loader before 
call_initPhase2 because in call_initPhase3, it will check if there is a 
custom system loader defined via the -Djava.system.loader property.

>
>> 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.
Yes, the simplification is possible. I also updated roots_oops_do() and 
oops_do() for the _java_platform_loader.

updated webrev: http://cr.openjdk.java.net/~ccheung/8185694/webrev.03/
>
> 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.
Below are couple of usages of is_system_class_loader() in ClassLoaderData:
// Returns true if this class loader data is for the system class loader.
bool ClassLoaderData::is_system_class_loader_data() const {
   return SystemDictionary::is_system_class_loader(class_loader());
}

// Returns true if this class loader data is one of the 3 builtin
// (boot, application/system or platform) class loaders. Note, the
// builtin loaders are not freed by a GC.
bool ClassLoaderData::is_builtin_class_loader_data() const {
   return (is_the_null_class_loader_data() ||
           SystemDictionary::is_system_class_loader(class_loader()) ||
           SystemDictionary::is_platform_class_loader(class_loader()));
}

I ran tests using custom loader and class unload tests like the 
following and they passed:

open/test/jdk/java/lang/ClassLoader/CustomSystemLoader/InitSystemLoaderTest.java
open/test/jdk/java/lang/System/
open/test/hotspot/jtreg/runtime/ClassUnload

thanks,
Calvin
>
> 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
>