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

[Lois Foltan]

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