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
