Prototype CGLIB replacement (Issue 1133) #1298

mcculls · 2020-03-13T02:59:33Z

This PR replaces CGLIB with prototype code to generate "enhancers" and "fast-classes".

I've tried this with a number of existing applications, however since this is new code and the whole area of proxying can be tricky (especially involving bridge methods) there will likely be tweaks and fixes needed :)

available for early testing, but not yet ready for review - more details to be added

This avoids the need for a marker interface, which in turn reduces the need for bridge classloaders. Since we store bridge classloaders in a weak cache, we're effectively just trading one for another.

…single type

a sorted list of strings. It assumes only those strings will be queried and therefore may produce false-positive results for strings not in the list. This trie will be used in the replacement FastClass/Enhancer.

…onsistency with other args

…st-class)

core/src/com/google/inject/internal/aop/FastClass.java

markmarch

Thanks for addressing all my comments.

This LGTM but I'll have sameb@ have a second look to make sure I haven't missed anything. Once Sam has LGTMed this PR, I'll work on submitting it internal (as the repo is not properly set up to auto accept PR).

…lass-loading

…ass-loading Note that package visibility wrt method intercepting is tested in BytecodeGenTest

…nd method parameters are also PUBLIC

sameb · 2020-06-01T19:47:17Z

core/src/com/google/inject/internal/DefaultConstructionProxyFactory.java

      }
-    } catch (net.sf.cglib.core.CodeGenerationException e) {
+    } catch (Exception | LinkageError e) {


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.

sameb · 2020-06-01T19:48:25Z

core/src/com/google/inject/internal/InternalFlags.java

+     */
+    ANONYMOUS,
+
+    /** Prefer Unsafe, but fall back to child class loaders if Unsafe is not available (Default) */


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.

sameb · 2020-06-01T19:49:11Z

core/src/com/google/inject/internal/ProviderMethod.java

+      try {
+        return (T) fastMethod.apply(instance, parameters);
+      } catch (Throwable e) {
+        throw new InvocationTargetException(e);


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

sameb · 2020-06-01T19:49:46Z

core/src/com/google/inject/internal/SingleMethodInjector.java

-            return fastClass.invoke(index, target, parameters);
+            try {
+              return fastMethod.apply(target, parameters);
+            } catch (Throwable e) {


(same q about wrapping w/ InvocationTargetException here)

sameb · 2020-06-01T19:50:11Z

core/src/com/google/inject/internal/SingleMethodInjector.java

          }
        };
      }
-    } catch (net.sf.cglib.core.CodeGenerationException e) {
+    } catch (Exception | LinkageError e) {


(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.)

sameb · 2020-06-01T19:50:48Z

core/src/com/google/inject/internal/aop/AbstractGlueGenerator.java

+        try {
+          // pass this signature's index into the table function to retrieve the invoker
+          return (BiFunction) invokerTable.invokeExact(signatureTable.applyAsInt(signature));
+        } catch (Throwable e) {


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.

sameb · 2020-06-01T19:51:18Z

core/src/com/google/inject/internal/aop/AbstractGlueGenerator.java

+  }
+
+  /**
+   * Generate trampoline that takes an index, along with a context object and array of argument


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.

sameb · 2020-06-01T19:51:49Z

core/src/com/google/inject/internal/aop/Enhancer.java

+  protected byte[] generateGlue(Collection<Executable> members) {
+    ClassWriter cw = new ClassWriter(COMPUTE_MAXS);
+
+    cw.visit(V1_8, PUBLIC | ACC_SUPER, proxyName, null, hostName, null);


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.

…atch JDK reflection behaviour

…atic final field

mkurz · 2020-06-26T09:45:45Z

@mcculls Scopes.java conflicts.

…ype-cglib-replacement

Replace CGLIB with custom code to generate "enhancers" and "fast-classes". Some user visible changes from using cglib: - intercepted method that has a return type of int but returns null from the interceptor will no longer be automatically converted to 0, instead a NullPointerException will be thrown. - Scope implementation can no longer check for circular proxy instance using CircularDependencyProxy marker class, instead should use Scopes.isCircularProxy - Depending on which custom class loading option is used, Guice enhanced class may no longer be mockable/spyable - Generated class name is slightly longer. Closes #1298 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=320433559

mcculls added 30 commits March 13, 2020 02:50

Use a weak cache to track circular proxy types.

6612a19

This avoids the need for a marker interface, which in turn reduces the need for bridge classloaders. Since we store bridge classloaders in a weak cache, we're effectively just trading one for another.

No need to use bridge classloader for native JDK proxies involving a …

6fb250b

…single type

Remove CGLIB dependency from the build

b916ace

Remove CGLIB related code and trim BytecodeGen back to the basics

c894d61

Immutable space-efficient trie that provides a fast index for

91a7ad4

a sorted list of strings. It assumes only those strings will be queried and therefore may produce false-positive results for strings not in the list. This trie will be used in the replacement FastClass/Enhancer.

Continue to flesh out BytecodeGen replacement

d7f8a91

Extract Visibility from old BytecodeGen

e9d9f59

Basic class definers

c5b5ebe

Skeleton bytecode generators

9dbe269

Support additional detail about enhancer targets

d1e9767

Determine enhanceable methods

aee104e

Minor cleanup

d202312

More cleanup

f126ecd

Restore custom class-loading flag for compatibility

a6b0ad5

Move Visibility back to BytecodeGen

c0521c9

Update test

5b51cc2

Update javadoc

6cfd827

Warn when intercepting package-private methods using CHILD class-loading

d47de13

Skip enhancing methods if they aren't visible using CHILD class loading

a957f05

Cleanup class definers

18ea2d6

Cleanup method resolution

8f276c8

Cleanup method resolution

877c033

Commentary

0a430a6

Track original bridge methods

03ffd81

Bind preferred ClassDefiner

d0c9c6c

Report minimum visibility supported by class definer

e39c316

Add ClassDefining entry-point

150911e

Add MethodResolving entry-point

a177a40

Naming consistency

c6210dd

Share enhancer glue according to which methods were enhanced

f7d1e38

mcculls added 6 commits May 11, 2020 00:16

method and target will have same name, but use target.getName() for c…

24c0ce4

…onsistency with other args

Document pros/cons of the different guice_custom_class_loading options

e193fc2

Remove unnecessary logic (intercepted setter already uses original fa…

d0f6176

…st-class)

Document which methods can be enhanced

ef33fad

Add pseudo-Java structures for the different glue classes

f1129c3

Add run of core tests with ANONYMOUS class-loading

5e5a2c0

google deleted a comment from mcculls May 12, 2020

markmarch reviewed May 13, 2020

View reviewed changes

core/src/com/google/inject/internal/aop/FastClass.java Outdated Show resolved Hide resolved

markmarch reviewed May 13, 2020

View reviewed changes

markmarch requested a review from sameb May 13, 2020 02:16

mcculls added 4 commits May 13, 2020 14:37

Add run of core tests with CHILD class-loading

fcbaadd

Exclude tests that rely on package access from being run with CHILD c…

f191a0e

…lass-loading

Small visibility changes to allow these tests to be run with CHILD cl…

ebfb5f0

…ass-loading Note that package visibility wrt method intercepting is tested in BytecodeGenTest

Fix canFastInvoke when using CHILD class-loading to check the class a…

215c153

…nd method parameters are also PUBLIC

mcculls mentioned this pull request May 31, 2020

Add a helper method to judge whether a given instance has been enhanced #1340

Closed

sameb reviewed Jun 1, 2020

View reviewed changes

mcculls added 5 commits June 5, 2020 21:58

javadoc

852942d

javadoc

6f29cde

Clarify we're wrapping exceptions with InvocationTargetException to m…

1386c48

…atch JDK reflection behaviour

Explain we only need to target Java8 for the generated helpers

774b89b

Improve FastClass performance: store adapted constructor handle in st…

f396f7e

…atic final field

mkurz mentioned this pull request Jun 26, 2020

Illegal reflective access by com.google.inject.internal.cglib.core.$ReflectUtils$1 #1133

Closed

Merge remote-tracking branch 'upstream/master' into issue-1133-protot…

d8ba81b

…ype-cglib-replacement

kevinb9n mentioned this pull request Jul 9, 2020

MOE Sync 2020-07-09 #1359

Merged

kevinb9n closed this in #1359 Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prototype CGLIB replacement (Issue 1133) #1298

Prototype CGLIB replacement (Issue 1133) #1298

mcculls commented Mar 13, 2020

markmarch left a comment

sameb Jun 1, 2020

mcculls Jun 5, 2020

sameb Jun 1, 2020

mcculls Jun 5, 2020

mcculls Jun 6, 2020

sameb Jun 1, 2020

mcculls Jun 6, 2020

mcculls Jun 6, 2020

sameb Jun 1, 2020

mcculls Jun 6, 2020

sameb Jun 1, 2020

mcculls Jun 6, 2020

sameb Jun 1, 2020

mcculls Jun 6, 2020

sameb Jun 1, 2020

mcculls Jun 6, 2020

sameb Jun 1, 2020

mcculls Jun 6, 2020

mcculls Jun 6, 2020

mkurz commented Jun 26, 2020

Prototype CGLIB replacement (Issue 1133) #1298

Prototype CGLIB replacement (Issue 1133) #1298

Conversation

mcculls commented Mar 13, 2020

markmarch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurz commented Jun 26, 2020