RFR (M): JDK-8159262: Walking PackageEntry Export and ModuleEntry Reads Must Occur Only When Neccessary And Wait Until ClassLoader's Aliveness Determined

[David Holmes]

Hi Lois,

I'm still unclear here - are you saying that during module 
initialization there is an "application class loader" used that is 
different to what may eventually be defined as the "system class 
loader"? I thought all module initialization was done by the boot loader. ??

I'm still not understanding why we have to care about / know about the 
actual type of the class loader ie 
jdk_internal_loader_ClassLoaders_AppClassLoader_klass() ??

Thanks,
David

On 23/06/2016 3:03 AM, Lois Foltan wrote:
>
> On 6/22/2016 12:31 PM, Coleen Phillimore wrote:
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8159262.1/webrev/src/share/vm/classfile/classLoaderData.cpp.udiff.html
>>
>>
>> The new function, is_system_class_loader_data are never used.  It
>> looks like the function was subsumed by is_builtin_class_loader_data()
>> which is fine.
>> The only other use of is_platform_class_loader_data is here: Should
>> this really be is_builtin_class_loader_data ?   The subtleties of the
>> distinctions are lost on me.
>>
>>   // Privileged code can use all annotations.  Other code silently
>> drops some.
>>   const bool privileged = loader_data->is_the_null_class_loader_data() ||
>> loader_data->is_platform_class_loader_data() ||
>>                           loader_data->is_anonymous();
>
> Hi Coleen,
> Thanks for reviewing!  That use of is_platform_class_loader_data() is in
> AnnotationCollector::annotation_index() which is outside the scope of
> this RFR so I am hesitant to change it now.
>
>>
>>
>> You might want to add for is_builtin_class_loader_data() a comment
>> that says that these are grouped together because they are the class
>> loaders that are not freed by GC.
> Good idea, will do.
>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8159262.1/webrev/src/share/vm/classfile/systemDictionary.cpp.udiff.html
>>
>>
>> Isn't is_system_class_loader The same as
>>
>> +bool SystemDictionary::is_system_class_loader(Handle class_loader) {
>> + return (is_platform_class_loader(class_loader) ||
>> + class_loader() == _java_system_loader);
>> +}
>> +
> No, the platform class loader is distinct from the application/system
> class loader.  And before SystemDictionary::_java_system_loader is
> initialized during bootstrapping, modules are defined to the built-in
> application class loader.  Thus is_system_class_loader must check for
> either:
>
> +  return (class_loader->klass() ==
> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() ||
>
> +          class_loader() == _java_system_loader);
> +}
>
>
> I think the confusion might be coming from the word "system", when one
> states "system class loaders" does that terminology imply either the
> application class loader or the platform class loader?  It doesn't, see
> http://download.java.net/java/jdk9/docs/api/java/lang/ClassLoader.html#getPlatformClassLoader,
> snipit follows:
>
> The Java run-time has the following built-in class loaders:
>
>  * Bootstrap class loader. It is the virtual machine's built-in class
>    loader, typically represented as|null|, and does not have a parent.
>  * Platform class loader
>
> <http://download.java.net/java/jdk9/docs/api/java/lang/ClassLoader.html#getPlatformClassLoader-->.
>
>    All/platform classes/are visible to the platform class loader that
>    can be used as the parent of a|ClassLoader|instance. Platform
>    classes include Java SE platform APIs, their implementation classes
>    and JDK-specific run-time classes that are defined by the platform
>    class loader or its ancestors.
>  * System class loader
>
> <http://download.java.net/java/jdk9/docs/api/java/lang/ClassLoader.html#getSystemClassLoader-->.
>
>    It is also known as/application class loader/and is distinct from
>    the platform class loader. The system class loader is typically used
>    to define classes on the application class path, module path, and
>    JDK-specific tools. The platform class loader is a parent or an
>    ancestor of the system class loader that all platform classes are
>    visible to it.
>
>>
>> ?  And a comment should say why checking against _java_system_loader
>> is done - ie that it is a custom system class loader configured
>> -Djava.system.class.loader.
> Good idea, will do.
>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8159262.1/webrev/src/share/vm/classfile/classLoaderData.cpp.udiff.html
>>
>>
>> Then is_builtin_class_loader_data should really be:
>>
>> +// Returns true if this class loader data is one of the 3 builtin
>> +// (boot, application/system or platform) class loaders.
>> +bool ClassLoaderData::is_builtin_class_loader_data() const {
>> + Handle classLoaderHandle = class_loader();
>> + return (is_the_null_class_loader_data() ||
>> + SystemDictionary::is_system_class_loader(classLoaderHandle));
>> +}
>>
>> ie. doesn't need to check the platform class loader and add to my
>> confusion.
> See explanation above why a call to is_system_class_loader and
> is_platform_class_loader must both be made.
>
>>
>> And I guess builtin_class_loader should really be named something
>> like, is_permanent_class_loader_data.   Or something like that because
>> that's really the distinction.
> Again, see snipit above.  They are "built-in" class loaders according to
> the documentation.
>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8159262.1/webrev/test/runtime/modules/ModuleStress/src/jdk.test/test/MainGC.java.html
>>
>>
>> the callback() function isn't used.
>
> Ah yes, the invocation here should be test.MainGC.callback().  I will fix.
> jdk.translet/translet/Main.java:        test.Main.callback();
>
>>
>> Otherwise, everything looks great.
>
> Thanks!
> Lois
>
>>
>> Coleen
>>
>>
>> On 6/22/16 10:51 AM, Lois Foltan wrote:
>>>
>>> On 6/22/2016 8:56 AM, Alan Bateman wrote:
>>>>
>>>>
>>>> On 22/06/2016 13:51, David Holmes wrote:
>>>>>
>>>>> How do you envisage any system classloader "dying" ??
>>>> The system class loader (be it the built-in application class loader
>>>> or a custom system class loader configured via
>>>> -Djava.system.class.loader) will/can never be GC'ed.
>>>> ClassLoader.getSystemClassLoader() always returns a reference to it
>>>> (for example).
>>> Ahh, ok, I was being overly cautious, so no opportunity to reset the
>>> system class loader dynamically at all?
>>> SystemDictionary::_java_class_loader is not initialized until after
>>> the module system initialization is complete.  Due to this I have
>>> updated my webrev to check for either the built-in application class
>>> loader or a custom system class loader and have renamed the method to
>>> is_system_class_loader().
>>>
>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8159262.1/webrev/
>>>
>>> Thanks,
>>> Lois
>>>
>>
>