RFR(S): 8185694: Replace SystemDictionaryShared::_java_platform_loader with SystemDictionary::is_platform_class_loader()
Jiangli Zhou
jiangli.zhou at oracle.com
Tue Oct 10 23:40:06 UTC 2017
I filed JDK-8189140 <https://bugs.openjdk.java.net/browse/JDK-8189140>.
Thanks,
Jiangli
> On Oct 10, 2017, at 2:49 PM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
>
>
>
> On 10/10/17, 2:38 PM, Jiangli Zhou wrote:
>> Hi Calvin,
>>
>>> On Oct 10, 2017, at 11:53 AM, Calvin Cheung <calvin.cheung at oracle.com <mailto:calvin.cheung at oracle.com>> wrote:
>>>
>>> I ran into some runtime issue when creating the _java_platform_loader before initPhase2.
>>> 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/ <http://cr.openjdk.java.net/%7Eccheung/8185694/webrev.04/>
>>
>> This looks good to me.
> Thanks for taking another look.
>>
>> SystemDictionary::compyte_java_loader() calls CDS_ONLY(SystemDictionaryShared::initialize()). The function name initialize() is misleading.
> The initialize() function is still initializing some classes. So I don't think the name is misleading.
>> Since you are touching the related code, could you please file a bug so we can clean up SystemDictionaryShared::initialize() API in the near future?
> If you don't mind, could you file one?
>
> thanks,
> Calvin
>>
>> Thanks,
>> Jiangli
>>
>>>
>>> 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 <mailto: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
>>>>>>>
>>
More information about the hotspot-runtime-dev
mailing list