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

[Calvin Cheung]

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

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