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

[David Holmes]

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