RFR 8171971: Fix timing bug in JVM management of package export lists

[harold seigel]

Thanks Seguei!

Harold


On 1/20/2017 9:47 AM, serguei.spitsyn at oracle.com wrote:
> +1
>
> Thanks,
> Serguei
>
>
> On 1/20/17 05:55, Lois Foltan wrote:
>> Looks good.
>> Lois
>>
>> On 1/19/2017 6:38 PM, harold seigel wrote:
>>> Hi,
>>>
>>> Here is an updated webrev containing the changes that you suggested.
>>>
>>> http://cr.openjdk.java.net/~hseigel/bug_8171971.2/webrev/index.html
>>>
>>> Could you take another look?
>>>
>>> Thanks, Harold
>>>
>>> On 1/19/2017 1:37 PM, harold seigel wrote:
>>>> Hi Karen, Lois,
>>>>
>>>> Thanks for the review!  Please see comments in-line.
>>>>
>>>> Harold
>>>>
>>>>
>>>> On 1/19/2017 11:54 AM, Karen Kinnear wrote:
>>>>> Harold,
>>>>>
>>>>> Thank you for doing this. And for the comments on the states.
>>>>>
>>>>> Looks good. I believe that the changes you have made have fixed 
>>>>> the timing holes. Thank you.
>>>>>
>>>>> 1) packageEntry.hpp: 145: set_unqual_exported()
>>>>> It would be helpful to have a comment that this explicitly sets 
>>>>> PKG_EXP_UNQUALIFIED and clears PKG_EXP_ALLUNNAMED.
>>>> Done
>>>>>
>>>>> I realized my concept of states is slightly different,
>>>>> with AllUnnamed, NamedExports and AllUnnamed+NamedExports being 
>>>>> different states. In future, when we rework this
>>>>> for performance, let’s revisit the states.
>>>> Agreed
>>>>>
>>>>>> On Jan 19, 2017, at 10:14 AM, Lois Foltan <lois.foltan at oracle.com 
>>>>>> <mailto:lois.foltan at oracle.com>> wrote:
>>>>>>
>>>>>>
>>>>>> On 1/18/2017 10:05 AM, harold seigel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this fix for the package export timing holes 
>>>>>>> discussed in JDK-8171971.  The fix reduces the number of 
>>>>>>> PackageEntry fields that are used to maintain a package's export 
>>>>>>> state and uses the Module_lock to protect all access to these 
>>>>>>> fields.
>>>>>>>
>>>>>>> Also, in cases where a package transitions from having qualified 
>>>>>>> exports to being unqualifiedly exported, it fixes the cleanup of 
>>>>>>> its qualified export list by removing the 
>>>>>>> _exported_pending_delete field and using just 
>>>>>>> is_unqual_exported() to determine when the qualified exports 
>>>>>>> list can be purged (at a safepoint).
>>>>>>>
>>>>>>> Open Webrev: 
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8171971/webrev/ 
>>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8171971/webrev/>
>>>>>>>
>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8171971
>>>>>>>
>>>>>>> The fix was tested with the hotspot, java/lang, java/util, 
>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, 
>>>>>>> RBT tier2 - tier5 tests on LinuxX64, and the colocated and 
>>>>>>> non-colocated NSK tests.
>>>>>>
>>>>>> Hi Harold,
>>>>>>
>>>>>> Looks good! A couple of comments in 
>>>>>> src/share/vm/classfile/packageEntry.hpp:
>>>>>>
>>>>>> - line #133, the method has_qual_exports_list().  A comment 
>>>>>> should be added ahead of this method to indicate that even if the 
>>>>>> _qualified_exports list is empty the package is still considered 
>>>>>> qualifiedly exported and a similar comment should be added to 
>>>>>> PackageEntry::purge_qualified_exports().  During a GC safepoint, 
>>>>>> it could be the scenario that all modules on a given package's 
>>>>>> qualified export list die, thus leaving an allocated but empty 
>>>>>> _qualified_exports field.  If the _qualified_exports 
>>>>>> GrowableArray was deleted in that scenario, then according to the 
>>>>>> new _exports_flag and a NULL value for _qualified_exports, the 
>>>>>> package would erroneously not be considered qualifiedly exported 
>>>>>> anymore.
>>>> Good point.  I added the comments.
>>>>> So not deleting the GrowableArray today is deliberate, because the 
>>>>> transition from qualifiedly exported to not exported is not 
>>>>> currently valid.
>>>>> Agreed, we may want to revisit that in future.
>>>>>
>>>>> 2) Harold, could you possibly add that state change to the comment 
>>>>> to the “A package cannot transition from”, i.e. after line 58?
>>>> Done
>>>>>> This can be reworked in the future to have the _exports_flag 
>>>>>> contain definitions for all possible states.  Relying on the 
>>>>>> state of whether _qualified_exports field is NULL or not can be 
>>>>>> problematic, but I think your changes do improve upon ensuring a 
>>>>>> PackageEntry maintains a valid state.
>>>>>>
>>>>>> - line #86 - please add a blank line for readability
>>>> okay
>>>>>> - line #139 & #143 please out the return expression in parentheses.
>>>> (okay)
>>>>>>
>>>>>> Thanks,
>>>>>> Lois
>>>>>>
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>
>>>>> thanks,
>>>>> Karen
>>>>>
>>>>
>>>
>>
>