RFR: 8159746: (proxy) Support for default methods

[Alan Bateman]

On Sun, 27 Sep 2020 17:09:03 GMT, Peter Levart <plevart 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

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.

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

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