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

[David Holmes]

One correction to the below ...

On 11/10/2017 11:22 AM, David Holmes wrote:
> Hi Calvin,
> 
> On 11/10/2017 4:53 AM, Calvin Cheung wrote:
>> I ran into some runtime issue when creating the _java_platform_loader 
>> before initPhase2.
> 
> Not really surprising.
> 
>> 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/
> 
> I think this work has gone off-track somewhat. You've ended up simply 
> caching the value of the platform loader, but that seems unrelated to 
> the bug synopsis:
> 
> "Replace SystemDictionaryShared::_java_platform_loader with 
> SystemDictionary::is_platform_class_loader()"
> 
> as you have not made that replacement! I expect that actual change is in 
> the soon-to-be-open AppCDS code - but such a change does not depend on 
> what you are doing here AFAICS. So why do we need the current change?

There is one case where the cached java_platform_loader() is used, but 
it is not a case where we are asking is_platform_class_loader().

David
-----

> I now have a somewhat better understanding of the issues here. We are 
> caching the system and (now) platform loaders after initPhase3 when 
> basically all of the core classes have been initialized, and the module 
> system etc. But during all of that initial classloading we will hit the 
> normal classloading logic that has to ask "is this the system/platform 
> loader?" and so the is_system/platform_loader() calls cannot use the 
> cached fields alone as they will not yet have been set!
> 
> In addition we have the complexity that when a custom system loader is 
> used, we initially have the built-in system loader acting as "the system 
> loader" until the real "system loader" has been created and installed - 
> hence the formulation:
> 
> bool SystemDictionary::is_system_class_loader(oop class_loader) {
>    if (class_loader == NULL) {
>      return false;
>    }
>    return (class_loader->klass() == 
> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() || 
> 
>         class_loader == _java_system_loader);
> }
> 
> For the platform loader there is only ever the built-in platform loader, 
> so it is simply:
> 
> bool SystemDictionary::is_platform_class_loader(oop class_loader) {
>    if (class_loader == NULL) {
>      return false;
>    }
>    return (class_loader->klass() == 
> SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass()); 
> 
> }
> 
> adding "|| class_loader == _java_platform_loader" would be pointless as 
> it could never be true if executed. So caching the value in 
> _java_platform_loader seems unnecessary.
> 
> I don't think we should be looking at any changes to the timing of the 
> system or platform classloader construction. We could look at caching 
> those values earlier in the initialization process (eg immediately after 
> the classloader instances are created) but I don't see any advantage in 
> doing so - it would not lead to any other code changes AFAICS.
> 
> David
> -----
> 
> 
>> 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 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
>>>>>>