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

[Ioi Lam]

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

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