On Fri, 23 Oct 2020 10:32:27 GMT, Peter Levart <plev...@openjdk.org> wrote:

>>> NewInvocationHandler/DefaultMethodInvoker feels a bit awkward. If I have it 
>>> right, to use it as a lambda expression to Proxy.newProxyInterface would 
>>> require casting to NewInvocationHandler. I also need to be careful when 
>>> using the invoker and not call it with a non-default method.
>> 
>> Yes it requires casting to `NewInvocationHandler` or  assign the lambda in a 
>> local variable.
>
> 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 JI
 T 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 cat
 ch/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.

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

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

Reply via email to