On Tue, 27 Oct 2020 01:23:07 GMT, Mandy Chung <mch...@openjdk.org> wrote:

>> Hi Mandy,
>> 
>> I think what matters here is a public surface area - what user sees. The API 
>> friendliness is the 1st aspect and very close to that is performance on the 
>> 2nd place. How complicated things are in the back is not unimportant, but 
>> very far from the leading two. I think this is how alternatives should be 
>> compared.
>> 
>> What I have done in my last attempt (https://github.com/mlchung/jdk/pull/4) 
>> is, admittedly, quite large from API surface. New `InvocationHandler2` 
>> interface taking additional `InvocationHandler2.SuperInvoker` callback + new 
>> overloaded static factory method `Proxy.newProxyInstance`. Additionally, 
>> internal checks needed to verify access at `newProxyInstance`-time are quite 
>> complicated since `InvocationHandler2` is a super-type of 
>> `InvocationHandler` and therefore the code has to check that the default 
>> implementation of `InvocationHandler.invoke(SuperInvoker, ...)` method is 
>> not overridden or else additional checks must be performed. This leads to 
>> complicated specification as to when access exceptions are thrown. From 
>> performance perspective, this variant showed least per-invocation overhead 
>> so far:
>> 
>> Benchmark               Mode  Cnt   Score   Error  Units
>> 
>> ProxyBench.implClass    avgt    5   3.777 ± 0.003  ns/op
>> ProxyBench.implProxy    avgt    5  20.282 ± 0.585  ns/op
>> ProxyBench.ppImplClass  avgt    5   3.752 ± 0.002  ns/op
>> ProxyBench.ppImplProxy  avgt    5  19.790 ± 0.335  ns/op
>> 
>> Your `DelegatingInvocationHandler` abstract class is nice and simple, but 
>> unfortunately not lambda-friendly. Will this hinder its use? I don't know. 
>> Alan says that this feature will mostly be for "experts", but even they like 
>> to use lambdas. The performance is quite OK as runtime per-invocation checks 
>> are not very expensive, just ~ 50% slower than my attempt above. Note that 
>> the allocation of 96 bytes per invocation is due to boxing 3 primitive 
>> argument ints into Integers which is unavoidable:
>> 
>> Benchmark                                                Mode  Cnt     Score 
>>      Error   Units
>> 
>> ProxyBench.implClass                                     avgt    5     3.765 
>> ±    0.028   ns/op
>> ProxyBench.implProxy                                     avgt    5    29.812 
>> ±    0.180   ns/op
>> ProxyBench.ppImplClass                                   avgt    5     3.705 
>> ±    0.110   ns/op
>> ProxyBench.ppImplProxy                                   avgt    5    31.209 
>> ±    0.900   ns/op
>> 
>> ProxyBench.implClass:·gc.alloc.rate.norm                 avgt    5    ≈ 10⁻⁶ 
>>               B/op
>> ProxyBench.implProxy:·gc.alloc.rate.norm                 avgt    5    96.004 
>> ±    0.001    B/op
>> ProxyBench.ppImplClass:·gc.alloc.rate.norm               avgt    5    ≈ 10⁻⁶ 
>>               B/op
>> ProxyBench.ppImplProxy:·gc.alloc.rate.norm               avgt    5    96.004 
>> ±    0.001    B/op
>> 
>> (NOTE: I noticed a possibility of a concurrent race condition in 
>> `Proxy.newProxyInstance` that could be used to fool the proxy interfaces 
>> access check - the array of interfaces is not defensively cloned)
>> 
>> Your `NewInvocationHandler` as a subinterface of `InvocationHandler` that 
>> overrides the InvocationHandler.invoke(Object proxy, ... and wires it to new 
>> abstract method taking additional DefaultMethodInvoker parameter seems 
>> similar to my last attempt, just subtype roles of old and new interfaces are 
>> reversed. This means it is easier to check whether the invocation handler 
>> has access to the super default methods or not (handler instanceof 
>> NewInvocationHandler) and adjust access checks in Proxy.newProxyInstance 
>> accordingly. But, as a consequence, the DefaultMethodInvoker instance has to 
>> be created for each invocation. You could also add an overloaded 
>> Proxy.newProxyInstance method to accept a lambda with 4 parameters 
>> (NewInvocationHandler), so this variant could be made user-friendly too, not 
>> requiring a cast. The performance is a little slower then abstract handler 
>> class. Also, the allocation of new instance of DefaultMethodInvoker per 
>> invocation seems unable to be optimized away by J
 IT escape analysis (maybe the DefaultMethodInvoker.invoke is to big to be 
inlined), so we have 16 additional bytes allocated per invocation:
>> 
>> Benchmark                                                Mode  Cnt     Score 
>>     Error   Units
>> 
>> ProxyBench.implClass                                     avgt    5     3.760 
>> ±   0.049   ns/op
>> ProxyBench.implProxy                                     avgt    5    34.260 
>> ±   0.993   ns/op
>> ProxyBench.ppImplClass                                   avgt    5     3.745 
>> ±   0.008   ns/op
>> ProxyBench.ppImplProxy                                   avgt    5    34.247 
>> ±   0.743   ns/op
>> 
>> ProxyBench.implClass:·gc.alloc.rate.norm                 avgt    5    ≈ 10⁻⁶ 
>>              B/op
>> ProxyBench.implProxy:·gc.alloc.rate.norm                 avgt    5   112.005 
>> ±   0.001    B/op
>> ProxyBench.ppImplClass:·gc.alloc.rate.norm               avgt    5    ≈ 10⁻⁶ 
>>              B/op
>> ProxyBench.ppImplProxy:·gc.alloc.rate.norm               avgt    5   112.005 
>> ±   0.001    B/op
>> 
>> 
>> From above 3 variants, I agree the `DelegatingInvocationHandler` abstract 
>> class seems to be most promising. But I have been thinking more about 
>> lambdas and have another idea that actually doesn't require any new types. 
>> Just an overloaded Proxy.newProxyInstance static method:
>> 
>> public static Object newProxyInstance(
>>     ClassLoader loader,
>>     Class<?>[] interfaces, 
>>     Function<? super InvocationHandler, ? extends InvocationHandler> 
>> handlerFactory
>> )
>> 
>> Here's the prototype:
>> 
>> https://github.com/plevart/jdk/commits/proxy-default-method-alt-api4
>> 
>> This is a patch against current https://github.com/openjdk/jdk, but all your 
>> latest "unrelated" changes (proxy class package/module) are included too. I 
>> can rebase this onto your 
>> https://github.com/mlchung/jdk/tree/proxy-default-method branch if required.
>> 
>> The performance of this one is the same as my previous alternative API 2:
>> 
>> Benchmark                                                Mode  Cnt     Score 
>>     Error   Units
>> 
>> ProxyBench.implClass                                     avgt    5     3.700 
>> ±   0.013   ns/op
>> ProxyBench.implProxy                                     avgt    5    20.264 
>> ±   0.551   ns/op
>> ProxyBench.ppImplClass                                   avgt    5     3.743 
>> ±   0.028   ns/op
>> ProxyBench.ppImplProxy                                   avgt    5    20.274 
>> ±   0.180   ns/op
>> 
>> ProxyBench.implClass:·gc.alloc.rate.norm                 avgt    5    ≈ 10⁻⁶ 
>>              B/op
>> ProxyBench.implProxy:·gc.alloc.rate.norm                 avgt    5    96.003 
>> ±   0.001    B/op
>> ProxyBench.ppImplClass:·gc.alloc.rate.norm               avgt    5    ≈ 10⁻⁶ 
>>              B/op
>> ProxyBench.ppImplProxy:·gc.alloc.rate.norm               avgt    5    96.003 
>> ±   0.001    B/op
>> 
>> So how does this compare to DelegatingInvocationHandler? It surely is more 
>> lambda-friendly and performance-wise it is the best we have so far. It is 
>> also not very complicated internally. The changes to ProxyGenerator are just 
>> to accomodate for new static final field holding the super-default-method 
>> InvocationHandler. Alternatively a ClassValue could be used for that without 
>> changes to ProxyGenerator albeit with somewhat bigger footprint per proxy 
>> class.
>> 
>> Now I would like to discuss one thing that relates to all implmenentations 
>> so far. Super default method invocation is currently modeled by the 
>> `Method.invoke` API, meaning that it throws `InvocationTargetException` 
>> wrapping the target exception thrown by the invoked method. But since this 
>> API is to be used from withing `InvocationHandler.invoke` implementations, 
>> such `InvocationTargetException` would typically need to be caught, 
>> unwrapped and its target exception re-thrown to honour the contract of the 
>> declared exceptions of the proxy method. If someone forgot to do that and 
>> leave `InvocationTargetException` to be thrown from 
>> `InvocationHandler.invoke` (and it would be very easy to forget that since 
>> that method is declared to throw `Throwable`), such 
>> `InvocationTargetException` would be wrapped again with 
>> `UndeclaredThrowableException` as per Proxy API behavior. So wouldn't it be 
>> better for the super-invoke API to throw exact exceptions that the invoked 
>> methods are throwing? No ca
 tch/re-throw unwrapped gymnastics would be necessary in the 
`InvocationHandler` implementations and forgetting to do that would not be 
possible. Such non-wrapping super-invoke API does not allow to distinguish 
between IllegalArgumentException(s) thrown by the invoked method and 
IllegalArgumentException(s) thrown from the API itself due to arguments 
incompatible with method signature though, but would one really want to 
distinguish them? Typically not. My alternative API 4 above uses existing 
`InvocationHandle.invoke` API for forwarding invocation to super default 
methods and this API already declares that it throws `Throwable` so it allows 
passing exceptions unwrapped, but current implementation still wraps exceptions 
thrown by the invoked method with IllegalArgumentException. If we decide that 
it is better to not wrap them, this can be trivially implemented.
>> 
>> Peter
>
> Hi Peter, 
> 
> Just a quick reply.  I like your idea for a `newProxyInstance` to take an 
> invocation handler factory.   I went through the security implication and 
> also looked into ways to avoid the need of `protected static 
> getSuperHandler(Lookup)` method.  I haven't checked the performance and also 
> updating the javadoc.   My prototype is in the proxy-default-method-4 branch. 
>   I will come back to this tomorrow and respond to the other comments you had.
> 
> This approach seems promising.

Hi Peter,

To capture what I considered for security: we need to ensure that the one who 
can invoke the default methods of the proxy interfaces of a proxy via the 
super-default-method invocation handler should have access.  So
- `newProxyInstance` will do access check to ensure that the caller class has 
access to all proxy interfaces at proxy creation time
- `getInvocationHandler` will do access check to ensure that the caller class 
has access to all proxy interfaces at proxy creation time.
- the owner of the proxy instance and invocation handler should protect the 
super-default-method handler passed to the invocation handler factory and make 
sure it's not leaked to malicious code.

Since this is a new factory method, I'm thinking to move the interfaces 
argument to the last so that it can be a vararg, like this:

public static Object newProxyInstance(
     Function<? super InvocationHandler, ? extends InvocationHandler> 
handlerFactory,
     ClassLoader loader,
     Class<?>... interfaces) throws IllegalAccessException

Since the current `getInvocationHandler` API does not throw 
`IllegalAccessException`, we have two options
a) have `getInvocationHandler` to throw an unchecked exception  wrapping `IAE` 
such as `IllegalCallerException` or `IllegalArgumentException`.  Or a new 
`UncheckedIllegalAccessException`.
b) add a new method `Proxy.invocationHandler(Object proxy) throws 
IllegalAccessException`.   `getInvocationHandler` will still need to throw an 
unchecked exception if the given proxy supports invocation of default methods.

Responding to Peter's comments:
> The changes to ProxyGenerator are just to accomodate for new static final 
> field holding the super-default-method InvocationHandler. Alternatively a 
> ClassValue could be used for that without changes to ProxyGenerator albeit 
> with somewhat bigger footprint per proxy class.

The static `newSuperHandler` method is just a bridge for the proxy class to 
create its super-default-method handler.  It's not really needed to expose as a 
public API.   Instead I pass the super-default-method to the private Proxy 
constructor and store it in a private final field.  Yes, a small footprint 
increase.  It's a tradeoff.

> So wouldn't it be better for the super-invoke API to throw exact exceptions 
> that the invoked methods are throwing? 

With this new API, `SuperInvocationHandler` should follow the spec of 
`InvocationHandler::invoke`:
Throwable - the exception to throw from the method invocation on the proxy 
instance. The exception's type must be assignable either to any of the 
exception types declared in the throws clause of the interface method or to the 
unchecked exception types java.lang.RuntimeException or java.lang.Error. If a 
checked exception is thrown by this method that is not assignable to any of the 
exception types declared in the throws clause of the interface method, then an 
UndeclaredThrowableException containing the exception that was thrown by this 
method will be thrown by the method invocation on the proxy instance.

In other words, I agree it does not need to wrap it with 
`InvocationTargetException`.  And `IllegalArgumentException` may be thrown by 
the invoked method and also thrown due to arguments mismatched with method 
signature.

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

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

Reply via email to