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

[Lois Foltan]

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