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
