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

[David Holmes]

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?

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