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

[Jiangli Zhou]

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