On Thu, 22 Oct 2020 17:07:55 GMT, Mandy Chung <mch...@openjdk.org> wrote:

>> Thanks for the update. Lots of challenges finding the right API as there are 
>> security and usability issues, not to mention performance and making it look 
>> like the support for default methods has always been there.
>> 
>> I've skimmed through both prototypes in proxy-method-default-3 branch, both 
>> seem to be secure.
>> 
>> 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.
>> 
>> DelegatingInvocationHandler feels more like a base implementation that I 
>> extend when I need help invoking default methods. Naming aside, it feels 
>> like it fits in better but with the downside that an instance can't be used 
>> as a functional interface. So I agree this one is a better. The users of 
>> this API will be super advanced developers and I would expect retrofitting 
>> existing code shouldn't be too hard for that are compiled to 16+.
>
>> 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 JIT 
 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 catch
 /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

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

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

Reply via email to