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

[Calvin Cheung]

On 10/10/17, 2:38 PM, Jiangli Zhou wrote:
> Hi Calvin,
>
>> On Oct 10, 2017, at 11:53 AM, Calvin Cheung <calvin.cheung at oracle.com 
>> <mailto:calvin.cheung at oracle.com>> wrote:
>>
>> I ran into some runtime issue when creating the _java_platform_loader 
>> before initPhase2.
>> I've filed the following to track the above issue:
>> https://bugs.openjdk.java.net/browse/JDK-8189120
>>
>> I'm going with the fix similar to version.02 - creating the system 
>> and platform loaders after initPhase3.
>> updated webrev:
>> http://cr.openjdk.java.net/~ccheung/8185694/webrev.04/ 
>> <http://cr.openjdk.java.net/%7Eccheung/8185694/webrev.04/>
>
> This looks good to me.
Thanks for taking another look.
>
> SystemDictionary::compyte_java_loader() calls 
> CDS_ONLY(SystemDictionaryShared::initialize()). The function name 
> initialize() is misleading.
The initialize() function is still initializing some classes. So I don't 
think the name is misleading.
> Since you are touching the related code, could you please file a bug 
> so we can clean up SystemDictionaryShared::initialize() API in the 
> near future?
If you don't mind, could you file one?

thanks,
Calvin
>
> Thanks,
> Jiangli
>
>>
>> thanks,
>> Calvin
>>
>> On 10/5/17, 10:38 PM, David Holmes wrote:
>>> On 6/10/2017 3:28 PM, Calvin Cheung wrote:
>>>> On 10/5/17, 6:33 PM, David Holmes wrote:
>>>>> Hi Coleen, Calvin,
>>>>>
>>>>> On 6/10/2017 6:54 AM, coleen.phillimore at oracle.com 
>>>>> <mailto:coleen.phillimore at oracle.com> wrote:
>>>>>> 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.
>>>>>
>>>>> The classloading delegation hierarchy (as of JDK 9) is:
>>>>> - boot loader (native)
>>>>>  - platform loader (built-in)
>>>>>    - system (aka application) loader (built-in)
>>>>>
>>>>> If the user specifies a custom class for the system loader then it 
>>>>> is loaded by an instance of the default system loader and becomes 
>>>>> a fourth level in the hierarchy, and it is then technically the 
>>>>> "system loader". None of these loaders, or the classes they load 
>>>>> can be unloaded.
>>>>>
>>>>> Which is presumably why the code checks both:
>>>>>
>>>>> 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 }
>>>>>
>>>>> because we need to treat both of these instances as the "system 
>>>>> loader" from a VM perspective? The same does not apply to the 
>>>>> platform loader.
>>>> We're obtaining the _java_system_loader after initPhase3 even 
>>>> before this change. Roughly, the calling sequence of initPhase3 is 
>>>> as follows:
>>>>
>>>> call_initPhase3()
>>>>     -> ClassLoader.initPhase3()
>>>>         -> ClassLoader.initSystemClassLoader()  which contains the 
>>>> following code:
>>>>
>>>>         String cn = System.getProperty("java.system.class.loader");
>>>>         if (cn != null) {
>>>>             try {
>>>>                 // custom class loader is only supported to be 
>>>> loaded from unnamed module
>>>>                 Constructor<?> ctor = Class.forName(cn, false, 
>>>> builtinLoader)
>>>> .getDeclaredConstructor(ClassLoader.class);
>>>>                 scl = (ClassLoader) ctor.newInstance(builtinLoader);
>>>>             } catch (Exception e) {
>>>>                 throw new Error(e);
>>>>             }
>>>>         } else {
>>>>             scl = builtinLoader;
>>>>         }
>>>>         return scl;
>>>>
>>>>         So initSystemClassLoader() will either return the built-in 
>>>> system loader or a custom loader if it exists.
>>>>
>>>> We use the getSystemClassLoader API to obtain the _java_system_loader:
>>>>
>>>>     public static ClassLoader getSystemClassLoader() {
>>>>         switch (VM.initLevel()) {
>>>>             case 0:
>>>>             case 1:
>>>>             case 2:
>>>>                 // the system class loader is the built-in app 
>>>> class loader during startup
>>>>                 return getBuiltinAppClassLoader();
>>>>             case 3:
>>>>                 String msg = "getSystemClassLoader should only be 
>>>> called after VM booted";
>>>>                 throw new InternalError(msg);
>>>>             case 4:
>>>>                 // system fully initialized
>>>>                 assert VM.isBooted() && scl != null;
>>>>                 SecurityManager sm = System.getSecurityManager();
>>>>                 if (sm != null) {
>>>>                     checkClassLoaderPermission(scl, 
>>>> Reflection.getCallerClass());
>>>>                 }
>>>>                 return scl;
>>>>             default:
>>>>                 throw new InternalError("should not reach here");
>>>>         }
>>>>     }
>>>>
>>>>     So the _java_system_loader will either be the built-in system 
>>>> loader or a custom loader. (case 4 in the above)
>>>>
>>>>     I don't quite understand why the check in line 184 is required?
>>>>     Is it for checking if a given class_loader is the same type 
>>>> (like an instanceof) as the built-in system loader?
>>>
>>> I believe it is checking if the loader is the built-in default 
>>> system loader, both to account for the case where/if 
>>> SystemDictionary::is_system_class_loader is called prior to 
>>> initPhase3 completing; and also to account for encountering the 
>>> default-built-in loader when the custom system loader delegates to it.
>>>
>>> I'd have to examine every call path to 
>>> SystemDictionary::is_system_class_loader to check all the details.
>>>
>>> David
>>> -----
>>>
>>>> thanks,
>>>> Calvin
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>> 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
>>>>>>
>