Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8159746: (proxy) Support for default methods #313

Closed
wants to merge 28 commits into from

Conversation

@mlchung
Copy link
Member

@mlchung mlchung commented Sep 22, 2020

This proposes a new static Proxy::invokeDefaultMethod method to invoke
the given default method on the given proxy instance.

The implementation looks up a method handle for invokespecial instruction
as if called from with the proxy class as the caller, equivalent to calling
X.super::m where X is a proxy interface of the proxy class and
X.super::m will resolve to the specified default method.

The implementation will call a private static proxyClassLookup(Lookup caller)
method of the proxy class to obtain its private Lookup. This private method
in the proxy class only allows a caller Lookup on java.lang.reflect.Proxy class
with full privilege access to use, or else IllegalAccessException will be
thrown.

This patch also proposes to define a proxy class in an unnamed module to
a dynamic module to strengthen encapsulation such that they are only
unconditionally exported from a named module but not open for deep reflective
access. This only applies to the case if all the proxy interfaces are public
and in a package that is exported or open.

One dynamic module is created for each class loader that defines proxies.
The change changes the dynamic module to contain another package (same
name as the module) that is unconditionally exported and is opened to
java.base only.

There is no change to the package and module of the proxy class for
the following cases:

  • if at least one proxy interface is non-public, then the proxy class is defined
    in the package and module of the non-public interfaces
  • if at least one proxy is in a package that is non-exported and non-open,
    if all proxy interfaces are public, then the proxy class is defined in
    a non-exported, non-open package of a dynamic module.

The spec change is that a proxy class used to be defined in an unnamed
module, i.e. in a exported and open package, is defined in an unconditionally
exported but non-open package. Programs that assume it to be open unconditionally
will be affected and cannot do deep reflection on such proxy classes.

Peter Levart contributed an initial prototype [1] (thanks Peter). I think
the exceptions could be simplified as more checking should be done prior to
the invocation of the method handle like checking the types of the arguments
with the method type. This approach avoids defining a public API
protected Proxy::$$proxyClassLookup$$ method. Instead it defines a
private static method that is restricted for Proxy class to use (by
taking a caller parameter to ensure it's a private lookup on Proxy class).

javadoc/specdiff:
http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/
http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041629.html


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Contributors

  • Peter Levart <plevart@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/313/head:pull/313
$ git checkout pull/313

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 22, 2020

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@mlchung The following labels will be automatically applied to this pull request: core-libs security.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Sep 23, 2020

/label remove security

@mlchung mlchung changed the title JDK-8159746: (proxy) Support for default methods 8159746: (proxy) Support for default methods Sep 23, 2020
@openjdk openjdk bot removed the security label Sep 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2020

@mlchung
The security label was successfully removed.

@openjdk openjdk bot added the rfr label Sep 23, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 23, 2020

@plevart
Copy link
Contributor

@plevart plevart commented Sep 27, 2020

Hi Mandy,
In general this looks good but one thing. The Proxy::invokeDefaultMethod does a lot of checks (creating derived method handles just to check parameters etc.) for every invocation and that means performance is not great. I created a benchmark to see how it compares with bytecode equivalent of invoking super default method: https://gist.github.com/plevart/eee13c50a91bdb61a79cf18a57a0af13 ...
The results are not great:

Benchmark             Mode  Cnt    Score    Error  Units
ProxyBench.implClass  avgt    5    3.709 ±  0.026  ns/op
ProxyBench.implProxy  avgt    5  926.215 ± 11.835  ns/op

...so I thought why not re-doing that part differently - do as many checks as possible just once and delegate checking of passed-in arguments to method handle chain itself instead of doing that in code with reflection. My attempt is expressed in the following commit on top of your PR:
plevart@9b13be1
(I couldn't create a PR against your forked jdk repo - I don't know why but Github doesn't list your fork in the popup to choose from, so I'm just pointing to commit in my fork). With this patch on top, the benchmark results are much better:

Benchmark             Mode  Cnt   Score   Error  Units
ProxyBench.implClass  avgt    5   3.777 ± 0.005  ns/op
ProxyBench.implProxy  avgt    5  27.579 ± 0.250  ns/op

This patch also changes a little the specification of thrown exception types. Since this is a new method, we could follow what method handles do. The choice among IllegalArgumentException, NullPointerException and ClassCastException seems pretty logical to me that way. In case you don't agree, there is a possibility to do it differently for the price of more complicated method handle chain but not necessarily slower performance.
So what do you think?
Regards, Peter

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 28, 2020

This is java.lang.reflect so throwing CCE for the mismatching parameter case would make it consistent with Method.invoke and maybe other methods. So the performance work is great and maybe see how far it can be brought by means of other MH combinators.

@plevart
Copy link
Contributor

@plevart plevart commented Sep 28, 2020

@AlanBateman you meant to say throwing CCE for mismatching parameter case would make it inconsistent with Method.invoke, right? That's easy to change. We just catch CCE (and NPE?) and re-throw them wrapped in IAE. We don't need to do that via MH combinators. Just directly in code. Performance will remain the same.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Sep 28, 2020

@AlanBateman you meant to say throwing CCE for mismatching parameter case would make it inconsistent with Method.invoke, right? That's easy to change. We just catch CCE (and NPE?) and re-throw them wrapped in IAE. We don't need to do that via MH combinators. Just directly in code. Performance will remain the same.

Right, Method::invoke throws IAE (not CCE).

@plevart
Copy link
Contributor

@plevart plevart commented Sep 29, 2020

So here's a patch on top of my last patch that reverts the exception behaviour back to be consistent with Method::invoke
mlchung#1

@mlchung
Copy link
Member Author

@mlchung mlchung commented Sep 29, 2020

So here's a patch on top of my last patch that reverts the exception behaviour back to be consistent with Method::invoke
mlchung#1

Thanks Peter. Looks good in general with a couple of comments. You can push to my local branch once you make any change per the feedback.

@plevart
Copy link
Contributor

@plevart plevart commented Nov 20, 2020

In my previous attempts when I was modifying the ProxyGenerator I noticed that generated proxy fields holding Method objects are just "static" but could be "static final". If they were "static final", JIT could constant-fold the Method instances. If this was combined with marking some fields in Method as @stable, this could improve optimization of code a bit further. I did this with the following patch:

Index: src/java.base/share/classes/java/lang/reflect/InvocationHandler.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/java.base/share/classes/java/lang/reflect/InvocationHandler.java	(revision 8352a20bf54a085a97d3f703b5dab482d3f9eccc)
+++ src/java.base/share/classes/java/lang/reflect/InvocationHandler.java	(date 1605869089804)
@@ -271,15 +271,10 @@
             throw new IllegalArgumentException("\"" + method + "\" is not a default method");
         }
         Class<?> intf = method.getDeclaringClass();
-        // access check if it is a non-public proxy interface or not unconditionally exported
-        if (!Modifier.isPublic(intf.getModifiers()) ||
-                !intf.getModule().isExported(intf.getPackageName())) {
-            // throw IAE if the caller class has no access to the default method
-            // same access check to Method::invoke on the default method
-            int modifiers = method.getModifiers();
-            Class<?> caller = Reflection.getCallerClass();
-            method.checkAccess(caller, intf, proxyClass, modifiers);
-        }
+        // access check
+        Class<?> caller = Reflection.getCallerClass();
+        int modifiers = method.getModifiers();
+        method.checkAccess(caller, intf, proxyClass, modifiers);
 
         MethodHandle mh = Proxy.defaultMethodHandle(proxyClass, method);
         // invoke the super method
Index: src/java.base/share/classes/java/lang/reflect/Method.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/java.base/share/classes/java/lang/reflect/Method.java	(revision 8352a20bf54a085a97d3f703b5dab482d3f9eccc)
+++ src/java.base/share/classes/java/lang/reflect/Method.java	(date 1605870445135)
@@ -31,6 +31,7 @@
 import jdk.internal.reflect.Reflection;
 import jdk.internal.vm.annotation.ForceInline;
 import jdk.internal.vm.annotation.IntrinsicCandidate;
+import jdk.internal.vm.annotation.Stable;
 import sun.reflect.annotation.ExceptionProxy;
 import sun.reflect.annotation.TypeNotPresentExceptionProxy;
 import sun.reflect.generics.repository.MethodRepository;
@@ -66,7 +67,7 @@
  * @since 1.1
  */
 public final class Method extends Executable {
-    private Class<?>            clazz;
+    @Stable private Class<?>    clazz;
     private int                 slot;
     // This is guaranteed to be interned by the VM in the 1.4
     // reflection implementation
@@ -74,7 +75,7 @@
     private Class<?>            returnType;
     private Class<?>[]          parameterTypes;
     private Class<?>[]          exceptionTypes;
-    private int                 modifiers;
+    @Stable private int         modifiers;
     // Generics and annotations support
     private transient String              signature;
     // generic info repository; lazily initialized
Index: src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java	(revision 8352a20bf54a085a97d3f703b5dab482d3f9eccc)
+++ src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java	(date 1605870027579)
@@ -490,7 +490,7 @@
         for (List<ProxyMethod> sigmethods : proxyMethods.values()) {
             for (ProxyMethod pm : sigmethods) {
                 // add static field for the Method object
-                visitField(Modifier.PRIVATE | Modifier.STATIC, pm.methodFieldName,
+                visitField(Modifier.PRIVATE | Modifier.STATIC | Modifier.FINAL, pm.methodFieldName,
                         LJLR_METHOD, null, null);
 
                 // Generate code for proxy method

...and this makes the following results:

Benchmark               Mode  Cnt   Score   Error  Units
ProxyBench.implClass    avgt    5   3.766 ± 0.040  ns/op
ProxyBench.implProxy    avgt    5  26.847 ± 0.626  ns/op
ProxyBench.ppImplClass  avgt    5   3.700 ± 0.017  ns/op
ProxyBench.ppImplProxy  avgt    5  26.322 ± 0.048  ns/op

But this can be changed as a follow-up patch that also takes care of Constructor and Field.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Nov 20, 2020

@plevart I'm okay with this slight performance improvement.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Nov 20, 2020

I have a fresh look at the different options we have explored (lots of challenges in finding the right API with security, usability and performance issues to consider). I agree with Remi that we should keep the design and API simple and makes it easier to invoke default methods with today's Proxy API. We can design a better Proxy API in the future.

The options we have explored are:

  1. static InvocationHandler::invokeDefault method
  2. abstract DelegatingInvocationHandler class with a protected invokeDefault method
  3. a new newProxyInstance factory method taking a function that produces an invocation handler with the ability to invoke a default method via a superHandler

(1) is very simple API but caller-sensitive. No other API change. Access check done at default method invocation time (which is consistent with the core reflection Method::invoke). It shares the caller class caching in Method::checkAccess which helps the performance. The performance overhead is slightly higher than (2) & (3) which does access check at proxy creation time.

(2) is simple and I like that the invokeDefault can be enforced to be invoked only by the proxy's invocation handler. However this requires more API changes (including newProxyInstance, getInvocationHandler, and new unchecked exception type). (3) is clever but a bit over-rotated (how Alan describes it) to allow it to be expressed in a lambda expression. If an invocation handler wants to save superHandler, it can't assign it to a final field in lambda and would need to workaround it writing to an array element.

I will go with option (1) - static invokeDefault method [1] unless there is any objection.

CSR: https://bugs.openjdk.java.net/browse/JDK-8253870

Here is the specdiff:
http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/

[1] http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/java.base/java/lang/reflect/InvocationHandler.html#invokeDefault(java.lang.Object,java.lang.reflect.Method,java.lang.Object...)

@mlchung
Copy link
Member Author

@mlchung mlchung commented Nov 20, 2020

/contributor add plevart

@openjdk
Copy link

@openjdk openjdk bot commented Nov 20, 2020

@mlchung
Contributor Peter Levart <plevart@openjdk.org> successfully added.

@forax
Copy link
Member

@forax forax commented Nov 20, 2020

Hi Mandy,
thanks for taking the time to explore all the different options,

The solution 1 is fine for me.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Nov 20, 2020

Thanks Remi.

@plevart
Copy link
Contributor

@plevart plevart commented Nov 20, 2020

I agree solution 1 is the best after all things considered.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 22, 2020

I"m also okay with with option 1, which is essentially the original proposal exception that it defines the method on InnovocationHandler instead of Proxy. Thanks Mandy and Peter for the prototypes, options, and iterations.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 26, 2020

@mlchung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8159746: (proxy) Support for default methods

Co-authored-by: Peter Levart <plevart@openjdk.org>
Reviewed-by: darcy, alanb, plevart

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 26, 2020
// the method's declaring class is a proxy interface
if (proxyInterfaces.contains(declaringClass))
return declaringClass;

Copy link
Contributor

@AlanBateman AlanBateman Nov 30, 2020

It might be beneficial for future maintainers if you could include a comment here on the search order.

Copy link
Member Author

@mlchung mlchung Nov 30, 2020

What about:

+        // find the first proxy interface that inherits the default method
+        // i.e. the declaring class of the default method is a superinterface
+        // of the proxy interface

*
* @return a lookup for proxy class of this proxy instance
*/
private static MethodHandles.Lookup proxyClassLookup(MethodHandles.Lookup caller, Class<?> proxyClass) {
Copy link
Contributor

@AlanBateman AlanBateman Nov 30, 2020

The method description could be a bit clearer. It invokes the proxy's proxyClassLookup method to get a Lookup on the proxy class ("this proxy instance" is just a bit confusing as proxyClassLookup is static).
I guess the caller parameter isn't really needed as it could be crated in proxyClassLookup.

Copy link
Member Author

@mlchung mlchung Nov 30, 2020

The caller parameter is just another level of defense. I updated as:

     /**
-     * Returns a Lookup object for the lookup class which is the class of this
-     * proxy instance.
+     * This method invokes the proxy's proxyClassLookup method to get a
+     * Lookup on the proxy class.
      *
      * @return a lookup for proxy class of this proxy instance
      */

Copy link
Contributor

@AlanBateman AlanBateman Nov 30, 2020

Much better, thanks!

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Overall, really good!

plevart
plevart approved these changes Dec 1, 2020
Copy link
Contributor

@plevart plevart left a comment

I looked again at the code and it seems good.

@mlchung
Copy link
Member Author

@mlchung mlchung commented Dec 1, 2020

/integrate

@openjdk openjdk bot closed this Dec 1, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 1, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2020

@mlchung Pushed as commit 56b15fb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants