Quantcast

RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

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

RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Peter Levart
Hi,

Module system implementation refresh 2017/2 (JDK-8173393) introduced new
API method AccessibleObject::canAccess which can be used to test if the
caller has access to the reflected member (with possible target object
argument for instance members). The implementation of this method, after
some parameter validation checks, delegates directly to
jdk.internal.reflect.Reflection::verifyMemberAccess. This is
sub-optimal. Co-located internal method AccessibleObject::checkAccess
also delegates to Reflection::verifyMemberAccess, but it also uses a
one-element cache of access-check decision, which greatly speeds up
repeated calls by the same caller to the same reflected member. The
cache could be shared between those two methods which would improve
performance of code idioms like this:

if (member.canAccess(target) || member.trySetAccessible()) {
   member.invoke(....);
     ...
} else {
    ...
}

Here's a proposed patch to improve this new method's performance:

http://cr.openjdk.java.net/~plevart/jdk9-dev/8177086_AccessibleObject.canAccess.cache/webrev.01/

The corresponding jira issue is:

https://bugs.openjdk.java.net/browse/JDK-8177086


I don't know whether this should go to jdk9 or perhaps later to jdk9u,
since this is new API that only came into existence 1 month ago and this
patch improves its implementation. Should It perhaps go to jake repo
1st? Presented patch is made against current jdk9/jdk repo.


Regards, Peter

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

Re: RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Alan Bateman
On 19/03/2017 17:52, Peter Levart wrote:

> Hi,
>
> Module system implementation refresh 2017/2 (JDK-8173393) introduced
> new API method AccessibleObject::canAccess which can be used to test
> if the caller has access to the reflected member (with possible target
> object argument for instance members). The implementation of this
> method, after some parameter validation checks, delegates directly to
> jdk.internal.reflect.Reflection::verifyMemberAccess. This is
> sub-optimal. Co-located internal method AccessibleObject::checkAccess
> also delegates to Reflection::verifyMemberAccess, but it also uses a
> one-element cache of access-check decision, which greatly speeds up
> repeated calls by the same caller to the same reflected member. The
> cache could be shared between those two methods which would improve
> performance of code idioms like this:
>
When integrating with the cache came up on jigsaw-dev a few work ago
then I think Mandy wanted to defer to it to JDK 10. However since
canAccess is new then having is be more efficient might be good, it just
mightn't be a P1/P2 for RDP2. Can you re-base the patch against jake as
has changed this code and then maybe we can figure out whether to pull
it into jake?

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

Re: RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Peter Levart
Hi Alan,

On 03/19/2017 08:03 PM, Alan Bateman wrote:

> On 19/03/2017 17:52, Peter Levart wrote:
>
>> Hi,
>>
>> Module system implementation refresh 2017/2 (JDK-8173393) introduced
>> new API method AccessibleObject::canAccess which can be used to test
>> if the caller has access to the reflected member (with possible
>> target object argument for instance members). The implementation of
>> this method, after some parameter validation checks, delegates
>> directly to jdk.internal.reflect.Reflection::verifyMemberAccess. This
>> is sub-optimal. Co-located internal method
>> AccessibleObject::checkAccess also delegates to
>> Reflection::verifyMemberAccess, but it also uses a one-element cache
>> of access-check decision, which greatly speeds up repeated calls by
>> the same caller to the same reflected member. The cache could be
>> shared between those two methods which would improve performance of
>> code idioms like this:
>>
> When integrating with the cache came up on jigsaw-dev a few work ago
> then I think Mandy wanted to defer to it to JDK 10. However since
> canAccess is new then having is be more efficient might be good, it
> just mightn't be a P1/P2 for RDP2. Can you re-base the patch against
> jake as has changed this code and then maybe we can figure out whether
> to pull it into jake?
>
> -Alan

No problem. Here it is (against tip of jake/jdk):

http://cr.openjdk.java.net/~plevart/jdk9-jake/AccessibleObject.canAccess_caching/webrev.02/


This patch changes a little when the stacktrace requested by
sun.reflect.debugModuleAccessChecks system property is printed. In
original code it is printed when the access (invocation, get/set) itself
fails as well as when AccessibleObject.canAccess() returns false. Is
that what was intended? Patched code only prints when the actual access
attempt fails and not when canAccess() returns false. If you want
canAccess() to also print stacktrace, I can add it to canAccess() method.

Tested with java/lang/reflect and all 80 tests pass.

Regards, Peter

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

Re: RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Jochen Theodorou
In reply to this post by Peter Levart
On 19.03.2017 18:52, Peter Levart wrote:
> Hi,
>
> Module system implementation refresh 2017/2 (JDK-8173393) introduced new
> API method AccessibleObject::canAccess which can be used to test if the
> caller has access to the reflected member (with possible target object
> argument for instance members).

not wanting to hijack the thread, but why is there no canAccess method
that takes a class argument to check if that class can access? Why
always depending so much on caller sensitive methods?

bye Jochen

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

Re: RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Alan Bateman
On 19/03/2017 22:47, Jochen Theodorou wrote:

>
> not wanting to hijack the thread, but why is there no canAccess method
> that takes a class argument to check if that class can access? Why
> always depending so much on caller sensitive methods?
This method is intended to be used in conjunction with
Constructor.newInstance, Method.invoke, Field.get, ...  The idiom in
Peter's mail combines this with trySetAccessible and makes it easy to
gracefully handle cases where access is not allowed.

JEP 274 added Lookup.accessClass and may be closer to what you need.

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

Re: RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Peter Levart
In reply to this post by Peter Levart


On 03/19/2017 11:42 PM, Peter Levart wrote:

> No problem. Here it is (against tip of jake/jdk):
>
> http://cr.openjdk.java.net/~plevart/jdk9-jake/AccessibleObject.canAccess_caching/webrev.02/
>
>
> This patch changes a little when the stacktrace requested by
> sun.reflect.debugModuleAccessChecks system property is printed. In
> original code it is printed when the access (invocation, get/set)
> itself fails as well as when AccessibleObject.canAccess() returns
> false. Is that what was intended? Patched code only prints when the
> actual access attempt fails and not when canAccess() returns false. If
> you want canAccess() to also print stacktrace, I can add it to
> canAccess() method.

...I was wrong about that claim. Original code does exactly what the
patch does - it only prints stack trace when checkAccess is called and
never when canAccess. As slowCheckMemberAccess (which printed the
stacktrace) was renamed to slowVerifyAccess (which is used from
canAccess too), the printing/throwing had to be moved to the checkAccess
method itself to preserve the behavior.

Peter

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

Re: RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Jochen Theodorou
In reply to this post by Alan Bateman
frankly I would migrate more from the normal reflection API to the
MethodHandles API instead of extending the Reflection API further, but I
see the use case and reasoning here. thanks.

bye Jochen

On 20.03.2017 10:04, Alan Bateman wrote:

> On 19/03/2017 22:47, Jochen Theodorou wrote:
>
>>
>> not wanting to hijack the thread, but why is there no canAccess method
>> that takes a class argument to check if that class can access? Why
>> always depending so much on caller sensitive methods?
> This method is intended to be used in conjunction with
> Constructor.newInstance, Method.invoke, Field.get, ...  The idiom in
> Peter's mail combines this with trySetAccessible and makes it easy to
> gracefully handle cases where access is not allowed.
>
> JEP 274 added Lookup.accessClass and may be closer to what you need.
>
> -Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Mandy Chung
In reply to this post by Peter Levart

> On Mar 19, 2017, at 3:42 PM, Peter Levart <[hidden email]> wrote:
>
> Hi Alan,
>>
>> When integrating with the cache came up on jigsaw-dev a few work ago then I think Mandy wanted to defer to it to JDK 10. However since canAccess is new then having is be more efficient might be good, it just mightn't be a P1/P2 for RDP2. Can you re-base the patch against jake as has changed this code and then maybe we can figure out whether to pull it into jake?
>>

Right I considered the optimization with the cache but deferred it as a follow-up issue.

>
> No problem. Here it is (against tip of jake/jdk):
>
> http://cr.openjdk.java.net/~plevart/jdk9-jake/AccessibleObject.canAccess_caching/webrev.02/
>

This patch looks good and nice speedup.  It may be good to pull in to jake for integration into jdk9 that will cover more testing.

Reflection.java
line 339-342: formatting nit: parameters in the signature consistent with the convention used in this file (see verifyMemberAccess method for example).

>
> This patch changes a little when the stacktrace requested by sun.reflect.debugModuleAccessChecks system property is printed. In original code it is printed when the access (invocation, get/set) itself fails as well as when AccessibleObject.canAccess() returns false. Is that what was intended? Patched code only prints when the actual access attempt fails and not when canAccess() returns false.

Good catch.  canAccess is a test method that does not change the access and no t printing the stack trace sounds right to me.

Thanks for this fix.
Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177086: java.lang.reflect.AccessibleObject::canAccess should share access cache with internal method ::checkAccess

Mandy Chung
In reply to this post by Peter Levart

> On Mar 20, 2017, at 2:58 AM, Peter Levart <[hidden email]> wrote:
>
>
>
> On 03/19/2017 11:42 PM, Peter Levart wrote:
>> No problem. Here it is (against tip of jake/jdk):
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-jake/AccessibleObject.canAccess_caching/webrev.02/
>>
>>
>> This patch changes a little when the stacktrace requested by sun.reflect.debugModuleAccessChecks system property is printed. In original code it is printed when the access (invocation, get/set) itself fails as well as when AccessibleObject.canAccess() returns false. Is that what was intended? Patched code only prints when the actual access attempt fails and not when canAccess() returns false. If you want canAccess() to also print stacktrace, I can add it to canAccess() method.
>
> ...I was wrong about that claim. Original code does exactly what the patch does - it only prints stack trace when checkAccess is called and never when canAccess. As slowCheckMemberAccess (which printed the stacktrace) was renamed to slowVerifyAccess (which is used from canAccess too), the printing/throwing had to be moved to the checkAccess method itself to preserve the behavior.

Looks like webrev.02 was updated to include what you described above.  Anyway, the patch looks okay to me.

Mandy
Loading...