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

[coleen.phillimore at oracle.com]

On 10/5/17 8:59 PM, Calvin Cheung wrote:
>
>
> 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

Great.   This change looks good.  A couple of small things:

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

*if (class_loader == NULL) {*
*return false;*
*}*

You probably don't have to check for NULL if you're simply checking for 
equality.   You may want to assert that 
compute_java_{system,platform}_loader do not return NULL.  I think that 
is an invariant.   I don't need to see another version and I'm glad you 
made this clearer.

I thought these were used by class unloading and determining that 
modules on the reads/exports list don't need to be walked.  If you run 
the test/hotspot/jtreg/runtime/modules tests, that would be good but 
they'll run under JPRT.

Thanks,
Coleen
>
> 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
>>