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/28/2016 11:07 AM, Zhengyu Gu wrote:
> Hi Lois,
>
> The comments in PackageEntry::delete_qualified_exports() suggests that 
> it is related to a transition from qualified exports to unqualified.

That is correct.  A package can transition from being qualifiedly 
exported, to unqualifiedly exported. Its exported scope is widened. A 
package can not transition from being unqualifiedly exported to all 
modules to being specifically qualified to one or more modules.

>
> It seems to me that, when overriding package's exports from qualified 
> to unqualified, ex. via JVM_AddModuleExportsToAll(), 
> delete_qualified_exports()
> should be called. I don't see it has anything to do with class unloading.

We chose not to delete the qualified exports at that point but to delay 
that until the next GC safepoint.  When a package does transition from 
being qualifiedly exported to unqualifiedly exported, its qualified 
exports list is moved to the _exported_pending_delete field which is 
then deleted later, via a call to 
PackageEntry::delete_qualified_exports().  That deletion of the 
_exported_pending_delete list does occur either during the purge walk or 
if the class loader that the package is defined to dies.

Thanks,
Lois

>
> Thanks,
>
> -Zhengyu
>
>
>
> On 06/28/2016 10:11 AM, Lois Foltan wrote:
>>
>> On 6/28/2016 9:28 AM, Zhengyu Gu wrote:
>>> Thanks for the changes.
>>>
>>> PackageEntry::delete_qualified_exports() has nothing to do with 
>>> class unloading, should it be handled in earlier loop? or it may get 
>>> lost.
>>
>> Hi Zhengyu,
>>
>> PackageEntry::delete_qualified_exports() have everything to do with 
>> class unloading and is a direct consequence of a loader dying.  When 
>> a class loader dies, its ModuleEntryTable and PackageEntryTable are 
>> deleted, see ClassLoaderData's destructor. In particular when a 
>> PackageEntryTable is deleted, each destructor is called for every 
>> package entry within that table which in return involves calling 
>> PackageEntry::delete_qualified_exports() if necessary.   That section 
>> of code has no bearing on this webrev and the changes in 
>> ClassLoaderDataGraph::do_unloading.
>>
>> Thanks,
>> Lois
>>
>>>
>>> -Zhengyu
>>>
>>> On 06/27/2016 04:50 PM, Lois Foltan wrote:
>>>>
>>>> On 6/27/2016 12:03 PM, Zhengyu Gu wrote:
>>>>> Hi Lois,
>>>>>
>>>>> I have a question:
>>>>>
>>>>> classLoaderData.cpp # 1004 - 1013
>>>>>
>>>>> If there is no class loader unloaded, (for example, _unloading == 
>>>>> NULL), do you still need to execute this purge loop?
>>>> Hi Zhengyu,
>>>>
>>>> Thank you for the review.  I have addressed your concern by moving 
>>>> the purge loop into the following conditional that only executes 
>>>> "if (seen_dead_loader)", nice catch.  This updated webrev also 
>>>> contains an improvement to the test ModuleStress.java to include a 
>>>> module defined to a custom system class loader, specified via 
>>>> -Djava.system.class.loader.
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8159262.2/webrev/
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Zhengyu
>>>>>
>>>>> On 06/22/2016 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
>>>>>>
>>>>>
>>>>
>>>
>>
>