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

[Coleen Phillimore]

On 6/22/16 1:03 PM, 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.

Thanks.  this would be good.
>
>>
>> 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);
> +}

Ok, I matched these two things when looking at the functions, when they 
are different.

+ return (class_loader->klass() == 
SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() ||


and

    return (class_loader->klass() == SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass());



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

Thanks.

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

Thanks for the snippet.  I didn't like my renaming anyway.  Thank you 
for adding the comments.

Coleen

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