RFR: 8159746: (proxy) Support for default methods

[Peter Levart]

On Tue, 22 Sep 2020 22:38:04 GMT, Mandy Chung <mchung at openjdk.org> wrote:

> 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

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:
https://github.com/plevart/jdk/commit/9b13be1064ec92ce8a25bda7e75f906e2e0b2978 (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

-------------

PR: https://git.openjdk.java.net/jdk/pull/313