On Tue, 22 Sep 2020 22:38:04 GMT, Mandy Chung <mch...@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
