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
