RFR (S) 8178336: Unnecessary SystemDictionary walk for Protection domain liveness

[coleen.phillimore at oracle.com]

Thank you Jiangli!
Coleen

On 4/13/17 12:48 AM, Jiangli Zhou wrote:
> Hi Coleen,
>
> Looks good.
>
> Thanks,
> Jiangli
>
>> On Apr 12, 2017, at 3:12 PM, coleen.phillimore at oracle.com wrote:
>>
>> Ioi,  Thank you for reviewing the code.  I've taken this opportunity to move the protectionDomainCache classes into files of their own with this change.  I also removed an unused function bucket_size() and removed a comment before ProtectionDomainCacheEntry about it having to go into the dictionary.hpp header file.
>>
>> I also added debug logging for removing protectiondomain entries, and verified entries were deleted, and ran some JDK jtreg protection domain tests.
>>
>> Can you review this new version?  Thanks!
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8178336.03/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8178336
>>
>> Thanks,
>> Coleen
>>
>> On 4/12/17 10:49 AM, Ioi Lam wrote:
>>> Looks good. Reviewed.
>>>
>>> Thanks
>>> - Ioi
>>>
>>> On 4/11/17 9:03 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> On 4/11/17 2:35 AM, Ioi Lam wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Thanks for doing this clean up. I was guiltily of writing the original code :-(
>>>>>
>>>>> A few questions:
>>>>>
>>>>> Why is this block of code moved and the comments dropped?
>>>>>
>>>>> 328 void Dictionary::oops_do(OopClosure* f) {
>>>>> 329   // Only the protection domain oops contain references into the heap. Iterate
>>>>> 330   // over all of them.
>>>>> 331   _pd_cache_table->oops_do(f);
>>>>> 332 }
>>>>> 333
>>>>>
>>>>> It would be better to make the changes in-place.
>>>> I didn't have to change Dictionary::oops_do and moved it to be near always_strong_oops_do(), so I shouldn't have removed the comment.  I moved it back but I don't like that it's separated from the other GC functions.   With my other change, it'll be closer (I think I'm going to have a merge conflict with myself).
>>>>> Also, have you validated that (either with an explicit test, or inside the debugger)
>>>>>
>>>>> [1] live protection domains in _pd_cache_table are properly relocated during GC?
>>>>> [2] dead protection domains are removed after class unloading?
>>>> I ran with runThese (which has lots of class loading and unloading) with logging for both oops_do and unlink functions (removing protection domain entries)  but I didn't realize that always_strong_oops_do is never called, so I deleted this function.
>>>>
>>>> New webrev (with dictionary::oops_do put back):
>>>>
>>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8178336.02/webrev/index.html
>>>>
>>>> Thanks!
>>>> Coleen
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>> On 4/11/17 4:18 AM, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: remove system dictionary walk and pass strong closure for !ClassUnloading
>>>>>>
>>>>>> See bug for more details:
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8178336.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8178336
>>>>>>
>>>>>> Tested with nightly tier2-5 tests and jprt (runs all GCs) and runThese with -XX:-ClassUnloading.
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen