RFR(S): 8185694: Replace SystemDictionaryShared::_java_platform_loader with SystemDictionary::is_platform_class_loader()
Ioi Lam
ioi.lam at oracle.com
Wed Oct 11 04:45:54 UTC 2017
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.
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
>>>>>>
More information about the hotspot-runtime-dev
mailing list