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

Thu Apr 13 13:47:05 UTC 2017

Looks good. Thanks Coleen!

Ioi

> coleen.phillimore at oracle.com 於 2017年4月13日 下午8:10 寫道：
> 
> 
> 
>> On 4/13/17 6:19 AM, Ioi Lam wrote:
>> Hi Coleen,
>> 
>> This is really good. Moving the protectionDomainCache out make it much easier to work on it separately, without getting stuck in what seems to be a million different versions of oops_do().
>> 
>> Everything looks fine. I just have one suggestion:
>> 
>> Every time I work on this code, I get confused about what ProtectionDomainEntry is. I need to refer to this comment (which I wrote :-)
>> 
>> 137   // Contains the set of approved protection domains that can access
>> 138   // this system dictionary entry.
>> 139   //
>> 140   // This protection domain set is a set of tuples:
>> 141   //
>> 142   // (InstanceKlass C, initiating class loader ICL, Protection Domain PD)
>> 143   //
>> 144   // [Note that C.protection_domain(), which is stored in the java.lang.Class
>> 145   // mirror of C, is NOT the same as PD]
>> 146   //
>> 147   // If such an entry (C, ICL, PD) exists in the table, it means that
>> 148   // it is okay for a class Foo to reference C, where
>> 149   //
>> 150   //    Foo.protection_domain() == PD, and
>> 151   //    Foo's defining class loader == ICL
>> 152   //
>> 153   // The usage of the PD set can be seen in SystemDictionary::validate_protection_domain()
>> 154   // It is essentially a cache to avoid repeated Java up-calls to
>> 155   // ClassLoader.checkPackageAccess().
>> 156   //
>> 157   ProtectionDomainEntry* _pd_set;
>> 
>> 
>> Maybe in the header that declares class ProtectionDomainEntry, add a comment to the effect:
>> 
>> "ProtectionDomainEntry doesn't do what you think it does -- it's NOT where InstanceKlass::protection_domain() stores its return value. See comments inside DictionaryEntry for details."
> 
> Yes, I agree, I'll add this comment to the header of protectionDomainCache.hpp:
> 
> // This class caches the approved protection domains that can access loaded classes.
> // Dictionary entry pd_set point to entries in this hashtable. Please refer
> // to dictionary.hpp pd_set for more information about how protection domain entries
> // are used.
> // This hashtable is walked for GC, not the entire system dictionary.
> 
> Thanks for all the help on this change.
> Coleen
>> 
>> Thanks
>> - Ioi
>> 
>> 
>>> On 4/13/17 6:12 AM, 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
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 