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

Thu Apr 13 12:10:33 UTC 2017

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