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

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jiangli Zhou
Hi Coleen,

I noticed there are two comments in Dictionary::do_unloading() refer to always_strong_oops_do. They should be updated as well.

150   // The placeholder array has been handled in always_strong_oops_do.
160       // Non-unloadable classes were handled in always_strong_oops_do
Thanks,
Jiangli

> On Apr 10, 2017, at 1:18 PM, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by coleen.phillimore
Hi Coleen,

On 11/04/2017 6:18 AM, [hidden email] wrote:
> Summary: remove system dictionary walk and pass strong closure for
> !ClassUnloading

Can't really comment on functional change but noticed this comment:

241   // Then iterate over the protection domain cache to apply the
closure on the
  242   // previously marked ones.
  243   _pd_cache_table->oops_do(blk);

is no longer accurate: there is nothing "before" for the "then" to
follow, and there is no previous marking done.

Thanks,
David

> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
In reply to this post by coleen.phillimore
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.

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?

Thanks
- Ioi

On 4/11/17 4:18 AM, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by Jiangli Zhou

Hi Jiangli,  Thank you for looking at this.

On 4/10/17 9:44 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
> I noticed there are two comments in Dictionary::do_unloading() refer
> to always_strong_oops_do. They should be updated as well.
>
> 150   // The placeholder array has been handled in always_strong_oops_do.
This comment makes no sense in this context so I removed it.
> 160       // Non-unloadable classes were handled in always_strong_oops_do

This comment doesn't make sense either in this context, and this whole
"strongly reachable" idea in this loop is odd.   If !ClassUnloading, all
of the classes are "strongly reachable" so we shouldn't really bother
going through this loop at all.  The other "strongly reachable" class is
the NULL class loader, where this "if" statement does make sense.  I've
made changes to this area in following work, so I'll remove this comment
for now.

Thanks!
Coleen

> Thanks,
> Jiangli
>
>> On Apr 10, 2017, at 1:18 PM, [hidden email]
>> <mailto:[hidden email]> 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 
>> <http://cr.openjdk.java.net/%7Ecoleenp/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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by David Holmes


On 4/11/17 2:05 AM, David Holmes wrote:

> Hi Coleen,
>
> On 11/04/2017 6:18 AM, [hidden email] wrote:
>> Summary: remove system dictionary walk and pass strong closure for
>> !ClassUnloading
>
> Can't really comment on functional change but noticed this comment:
>
> 241   // Then iterate over the protection domain cache to apply the
> closure on the
>  242   // previously marked ones.
>  243   _pd_cache_table->oops_do(blk);

Thanks - I tried to follow and update all the comments, but apparently
missed some:

   // Follow oops that are in the protection domain cache table

This is what I changed it to.
Coleen

>
> is no longer accurate: there is nothing "before" for the "then" to
> follow, and there is no previous marking done.
>
> Thanks,
> David
>
>> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by Ioi Lam


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, [hidden email] 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


On 4/11/17 9:03 AM, [hidden email] 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 :-(

I should point out that I reviewed it :)
Coleen

>>
>> 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, [hidden email] 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
In reply to this post by coleen.phillimore
Looks good. Reviewed.

Thanks
- Ioi

On 4/11/17 9:03 PM, [hidden email] 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, [hidden email] 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
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, [hidden email] 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, [hidden email] 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
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
In reply to this post by coleen.phillimore
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, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


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, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by Jiangli Zhou

Thank you Jiangli!
Coleen

On 4/13/17 12:48 AM, Jiangli Zhou wrote:

> Hi Coleen,
>
> Looks good.
>
> Thanks,
> Jiangli
>
>> On Apr 12, 2017, at 3:12 PM, [hidden email] 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, [hidden email] 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, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
In reply to this post by coleen.phillimore
Looks good. Thanks Coleen!

Ioi

> [hidden email] 於 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, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>>>
>>>>>
>>>>
>>>
>>
>

Loading...