do we have any narrower type we can catch here, to avoid over-catching exceptions & suppressing real errors?

(same elsewhere we catch Exception or Throwable)

This is meant to catch any exceptions that occurred while creating the fast constructor (not invocation as that happens inside FastClassProxy) and fall-through to use JDK reflection instead. CGLIB wrapped anything extending Exception inside CodeGenerationException so this just widens it to include linkage errors which can sometimes be thrown during class-loading. We'd want to fall back to JDK reflection in that case too.

We could consider logging the exception, but we'd still want to fall back to JDK reflection when we can't create the fast constructor.

can we clarify how "Prefer Unsafe" differs from "Unsafe.defineAnonymousClass"? what Unsafe method is it using? (or is it defineAnonymousClass?)

same in OFF's docs

For the general Unsafe case we use Unsafe.defineAnonymousClass to get access to the host classloader's defineClass method. Once we have access we then define classes using that classloader. We only use defineAnonymousClass to define all classes when guice_custom_class_loading is set to ANONYMOUS.

I'll expand on this in the javadoc.

6f29cde

should the the FastClass/Enhancer code prewrap in InvocationTargetExceptions?

otherwise it looks like all callers need to remember to do that, and if they don't then (i'm assuming) checked exceptions from the user code can slip out? unless it's throwing the user-errors inside of some kind of runtime exception (to make it work w/ BiFunction, in which case do we need to unwrap from that?)

(also it looks like IllegalAccessException isn't possible anymore?)

same comment throughout where we do this.

Enhanced classes don't pre-wrap exceptions with InvocationTargetException because they need to match the behaviour of the original type as closely as possible. To to keep class generation simple, fast-classes also don't do any pre-wrapping. In other words the fast-class behaves as if we were calling the original class directly (from an exception PoV).

Instead we apply InvocationTargetException to the few fast-constructor/fast-method places that need it to match JDK reflection behaviour. (This behaviour is checked by some internal tests.) This is also closer to the calling code that unwraps the invocation exception and reports the original cause, rather than have it hidden inside the generated code.

Wrt. checked exceptions from user code: those are propagated from the BiFunction by using a common trick to get the Java compiler to treat them as unchecked (all exceptions are unchecked from the perspective of the JVM.) This means the generated code doesn't need to do any wrapping itself, only the couple of places that need to match JDK reflection behaviour.

Also wrt. IllegalAccessException - that doesn't apply to the 'fast-class' case because we only use 'fast' invocation when we know we have enough access to benefit from it, and the generated code deliberately gives enough access to Guice to use the generated invoker.

added a bit of javadoc in 1386c48 and removed IllegalAccessException for fast-invocation

(same q about wrapping w/ InvocationTargetException here)

See above

(same q about catching narrower exceptions here)

See above - we want to fall back to JDK reflection if any exception or class-loading linkage error occurs when creating the fast-method (note this doesn't cover the actual invocation.)

should WrongMethodTypeException be handled separately here? javadoc describes that as thrown when "if the target's type is not identical with the caller's symbolic type descriptor", as opposed to every other kind of exception, which is just propagated from the underlying call.

same below

I don't believe we need to handle WrongMethodTypeException separately here.

does "and array of argument objects" imply there's a penalty for using the trampoline, in order to allocate the array & stuff the args into it? is this a change from how FastClass or Enhancer worked?

[have we done any benchmarks on the old vs new AOP approaches?]

For fast-invocation we are given an array of objects (which gets passed to either the fast-invoker or JDK reflection.)

Likewise for enhanced types, any intercepted methods need to convert their original arguments into an array of objects so they can be passed through an InvocationHandler and onto the method interceptor as MethodInvocation details. Methods that don't have any interceptors are not enhanced.

So the object array is a necessary detail of any solution (either having it passed in or expecting it to be provided for interception purposes.)

Re: benchmarks, I've run various tests that show performance is better using the new approach. For method interception the improvement per-invocation is small, but there's a much larger benefit when it comes to the up-front cost of interception when creating new injectors.

add a comment on why we're using v1_8 compatible classes (as opposed to anything older or newer)?

also: does this limit our ability to interact with host code that uses newer JDK features?

also: we use the constant in a bunch of places, should we extract to a shared constant so we can increment throughout the codebase more easily? or is that not a meaningful thing to do?

We use V1_8 as a minimum because that's the base version that Guice currently supports. The bytecode generated for the fast-class and enhancer glue is simple and doesn't need anything more than Java8. This is because we're not copying the actual method code, only generating invokers that match the expected signature.

I'd prefer not to use a shared constant here because it makes it harder to see what level we're targeting. Also in the future we may decide we want to change the version for one of the cases, but not the others.

774b89b