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

[Coleen Phillimore]

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();


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.

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);
+}
+

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

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.

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.

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.

Otherwise, everything looks great.

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
>