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

[David Holmes]

Hi Ioi,

On 11/10/2017 2:45 PM, Ioi Lam wrote:
> 
> 
> On 10/10/17 6:22 PM, 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?
>>
> 
> David, I filed JDK-8185694 as a clean-up task, so we can avoid using the 
> field _java_platform_loader directly, and replace that with a function 
> call.
> 
> The replacement (from the field to the function call) is currently in 
> the closed code. This code will be moved to open soon (see the JEP: 
> JDK-8185996).
> 
> As I mentioned in JDK-8188791 (the actual task for moving the code), we 
> will separate the "moving the code" step vs the "cleaning up the code" 
> steps. Otherwise it will be very hard to manage concurrent modifications 
> to the related source files. JDK-8185694 (_java_platform_loader) is just 
> one of those "cleaning up" steps, which happens to be done before the 
> code is actually moved.

Yes you are right - I needed to look back at Calvin's original RFR which 
simply states:

"The open code change for this fix involves adding the 
_java_platform_loader to the SystemDictionary class. "

which is all this change actually does.

Things got muddled with all the suggestions to change the definitions of 
is_platform/system_class_loader() - combined with the observation that 
although the bug is titled "Replace 
SystemDictionaryShared::_java_platform_loader with 
SystemDictionary::is_platform_class_loader()" that doesn't actually 
require the change being implemented here.

I'll also note that there may be subtle differences in behaviour in 
using SystemDictionary::is_platform_class_loader() if you currently call 
it before the platform loader would have been cached.

Thanks,
David

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