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

[coleen.phillimore at oracle.com]

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