master moved from 4daa205dc4fe to 4a4d8257ed41
2 new revisions:
Revision: 409e0f578b62
Author: Sam Berlin <[email protected]>
Date: Sat May 10 14:34:16 2014 UTC
Log: Try to use cglibs FastClass to invoke @Provides methods, it's
faster!...
http://code.google.com/p/google-guice/source/detail?r=409e0f578b62
Revision: 4a4d8257ed41
Author: Sam Berlin <[email protected]>
Date: Sat May 10 14:34:44 2014 UTC
Log: Block when transferring request scope instead of checking owners
& thr...
http://code.google.com/p/google-guice/source/detail?r=4a4d8257ed41
==============================================================================
Revision: 409e0f578b62
Author: Sam Berlin <[email protected]>
Date: Sat May 10 14:34:16 2014 UTC
Log: Try to use cglibs FastClass to invoke @Provides methods, it's
faster!
Here is a caliper benchmark to demonstrate the difference.
https://caliper.googleplex.com/runs/2d349fec-2742-45e1-b6e5-16997c522387#r:scenario.benchmarkSpec.methodName,scenario.benchmarkSpec.parameters.strategy
This should save about 200-300 ns per method invocation and about 224 bytes
(over 4 objects) of allocations for each invocation.
The cost of this (of course) is greater permgen usage and potentially
slower startup/injector creation due to the class generation.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=66364901
http://code.google.com/p/google-guice/source/detail?r=409e0f578b62
Modified:
/core/src/com/google/inject/internal/ProviderMethod.java
/core/src/com/google/inject/internal/ProviderMethodsModule.java
/core/test/com/google/inject/spi/ProviderMethodsTest.java
=======================================
--- /core/src/com/google/inject/internal/ProviderMethod.java Mon Mar 10
16:52:01 2014 UTC
+++ /core/src/com/google/inject/internal/ProviderMethod.java Sat May 10
14:34:16 2014 UTC
@@ -17,12 +17,14 @@
package com.google.inject.internal;
import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.inject.Binder;
import com.google.inject.Exposed;
import com.google.inject.Key;
import com.google.inject.PrivateBinder;
import com.google.inject.Provider;
+import com.google.inject.internal.BytecodeGen.Visibility;
import com.google.inject.internal.util.StackTraceElements;
import com.google.inject.spi.BindingTargetVisitor;
import com.google.inject.spi.Dependency;
@@ -35,6 +37,7 @@
import java.lang.annotation.Annotation;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
import java.util.List;
import java.util.Set;
@@ -43,12 +46,44 @@
*
* @author [email protected] (Jesse Wilson)
*/
-public class ProviderMethod<T> implements ProviderWithExtensionVisitor<T>,
HasDependencies,
+public abstract class ProviderMethod<T> implements
ProviderWithExtensionVisitor<T>, HasDependencies,
ProvidesMethodBinding<T> {
+
+ /**
+ * Creates a {@link ProviderMethod}.
+ *
+ * <p>Ideally, we will use {@link FastClass} to invoke the actual
method, since it is
+ * significantly faster. However, this will fail if the method is
{@code private} or
+ * {@code protected}, since fastclass is subject to java access policies.
+ */
+ static <T> ProviderMethod<T> create(Key<T> key, Method method, Object
instance,
+ ImmutableSet<Dependency<?>> dependencies, List<Provider<?>>
parameterProviders,
+ Class<? extends Annotation> scopeAnnotation) {
+ int modifiers = method.getModifiers();
+ /*if[AOP]*/
+ if (!Modifier.isPrivate(modifiers)
&& !Modifier.isProtected(modifiers)) {
+ try {
+ // We use an index instead of FastMethod to save a stack frame.
+ return new FastClassProviderMethod<T>(
+ key, method, instance, dependencies, parameterProviders,
scopeAnnotation);
+ } catch (net.sf.cglib.core.CodeGenerationException e) {/*
fall-through */}
+ }
+ /*end[AOP]*/
+
+ if (!Modifier.isPublic(modifiers) ||
+ !Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
+ method.setAccessible(true);
+ }
+
+ return new ReflectionProviderMethod<T>(
+ key, method, instance, dependencies, parameterProviders,
scopeAnnotation);
+ }
+
+ protected final Object instance;
+ protected final Method method;
+
private final Key<T> key;
private final Class<? extends Annotation> scopeAnnotation;
- private final Object instance;
- private final Method method;
private final ImmutableSet<Dependency<?>> dependencies;
private final List<Provider<?>> parameterProviders;
private final boolean exposed;
@@ -56,7 +91,7 @@
/**
* @param method the method to invoke. It's return type must be the same
type as {@code key}.
*/
- ProviderMethod(Key<T> key, Method method, Object instance,
+ private ProviderMethod(Key<T> key, Method method, Object instance,
ImmutableSet<Dependency<?>> dependencies, List<Provider<?>>
parameterProviders,
Class<? extends Annotation> scopeAnnotation) {
this.key = key;
@@ -66,8 +101,6 @@
this.method = method;
this.parameterProviders = parameterProviders;
this.exposed = method.isAnnotationPresent(Exposed.class);
-
- method.setAccessible(true);
}
public Key<T> getKey() {
@@ -110,9 +143,8 @@
}
try {
- // We know this cast is safe becase T is the method's return type.
@SuppressWarnings({ "unchecked", "UnnecessaryLocalVariable" })
- T result = (T) method.invoke(instance, parameters);
+ T result = (T) doProvision(parameters);
return result;
} catch (IllegalAccessException e) {
throw new AssertionError(e);
@@ -120,6 +152,10 @@
throw Exceptions.rethrowCause(e);
}
}
+
+ /** Extension point for our subclasses to implement the provisioning
strategy. */
+ abstract Object doProvision(Object[] parameters)
+ throws IllegalAccessException, InvocationTargetException;
public Set<Dependency<?>> getDependencies() {
return dependencies;
@@ -156,4 +192,61 @@
// (We need to call equals, so we do. But we can avoid hashCode.)
return Objects.hashCode(method);
}
+
+ /*if[AOP]*/
+ /**
+ * A {@link ProviderMethod} implementation that uses {@link
net.sf.cglib.reflect.FastClass#invoke}
+ * to invoke the provider method.
+ */
+ private static final class FastClassProviderMethod<T> extends
ProviderMethod<T> {
+ final net.sf.cglib.reflect.FastClass fastClass;
+ final int methodIndex;
+
+ FastClassProviderMethod(Key<T> key,
+ Method method,
+ Object instance,
+ ImmutableSet<Dependency<?>> dependencies,
+ List<Provider<?>> parameterProviders,
+ Class<? extends Annotation> scopeAnnotation) {
+ super(key, method, instance, dependencies, parameterProviders,
scopeAnnotation);
+ // We need to generate a FastClass for the method's class, not the
object's class.
+ this.fastClass =
+ BytecodeGen.newFastClass(method.getDeclaringClass(),
Visibility.forMember(method));
+ // Use the Signature overload of getIndex because it properly uses
return types to identify
+ // particular methods. This is normally irrelevant, except in the
case of covariant overrides
+ // which java implements with a compiler generated bridge method to
implement the override.
+ this.methodIndex = fastClass.getIndex(
+ new net.sf.cglib.core.Signature(
+ method.getName(),
org.objectweb.asm.Type.getMethodDescriptor(method)));
+ Preconditions.checkArgument(this.methodIndex >= 0,
+ "Could not find method %s in fast class for class %s",
+ method,
+ method.getDeclaringClass());
+ }
+
+ @Override public Object doProvision(Object[] parameters)
+ throws IllegalAccessException, InvocationTargetException {
+ return fastClass.invoke(methodIndex, instance, parameters);
+ }
+ }
+ /*end[AOP]*/
+
+ /**
+ * A {@link ProviderMethod} implementation that invokes the method using
normal java reflection.
+ */
+ private static final class ReflectionProviderMethod<T> extends
ProviderMethod<T> {
+ ReflectionProviderMethod(Key<T> key,
+ Method method,
+ Object instance,
+ ImmutableSet<Dependency<?>> dependencies,
+ List<Provider<?>> parameterProviders,
+ Class<? extends Annotation> scopeAnnotation) {
+ super(key, method, instance, dependencies, parameterProviders,
scopeAnnotation);
+ }
+
+ @Override Object doProvision(Object[] parameters) throws
IllegalAccessException,
+ InvocationTargetException {
+ return method.invoke(instance, parameters);
+ }
+ }
}
=======================================
--- /core/src/com/google/inject/internal/ProviderMethodsModule.java Fri
Jul 8 00:34:16 2011 UTC
+++ /core/src/com/google/inject/internal/ProviderMethodsModule.java Sat May
10 14:34:16 2014 UTC
@@ -123,7 +123,7 @@
binder.addError(message);
}
- return new ProviderMethod<T>(key, method, delegate,
ImmutableSet.copyOf(dependencies),
+ return ProviderMethod.create(key, method, delegate,
ImmutableSet.copyOf(dependencies),
parameterProviders, scopeAnnotation);
}
=======================================
--- /core/test/com/google/inject/spi/ProviderMethodsTest.java Mon Mar 10
16:52:01 2014 UTC
+++ /core/test/com/google/inject/spi/ProviderMethodsTest.java Sat May 10
14:34:16 2014 UTC
@@ -33,6 +33,8 @@
import com.google.inject.Module;
import com.google.inject.Provides;
import com.google.inject.Singleton;
+import com.google.inject.Stage;
+import com.google.inject.internal.InternalFlags;
import com.google.inject.internal.ProviderMethod;
import com.google.inject.internal.ProviderMethodsModule;
import com.google.inject.name.Named;
@@ -44,6 +46,7 @@
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
+import java.util.Collection;
import java.util.List;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
@@ -355,7 +358,7 @@
ProviderInstanceBinding binding = (ProviderInstanceBinding) element;
javax.inject.Provider provider = binding.getUserSuppliedProvider();
- assertEquals(ProviderMethod.class, provider.getClass());
+ assertTrue(ProviderMethod.class.isAssignableFrom(provider.getClass()));
assertEquals(methodsObject, ((ProviderMethod) provider).getInstance());
assertSame(provider, binding.getProviderInstance());
}
@@ -452,4 +455,192 @@
throw new IllegalStateException("unexpected visit of: " + binding);
}
}
+
+ public void testProvidesMethodVisibility() {
+ Injector injector = Guice.createInjector(new VisibilityModule());
+
+ assertEquals(42, injector.getInstance(Integer.class).intValue());
+ assertEquals(42L, injector.getInstance(Long.class).longValue());
+ assertEquals(42D, injector.getInstance(Double.class).doubleValue());
+ assertEquals(42F, injector.getInstance(Float.class).floatValue());
+ }
+
+ private static class VisibilityModule extends AbstractModule {
+ @Override protected void configure() {}
+
+ @SuppressWarnings("unused")
+ @Provides Integer foo() {
+ return 42;
+ }
+
+ @SuppressWarnings("unused")
+ @Provides private Long bar() {
+ return 42L;
+ }
+
+ @SuppressWarnings("unused")
+ @Provides protected Double baz() {
+ return 42D;
+ }
+
+ @SuppressWarnings("unused")
+ @Provides public Float quux() {
+ return 42F;
+ }
+ }
+
+ public void testProvidesMethodInheritenceHierarchy() {
+ try {
+ Guice.createInjector(new Sub1Module(), new Sub2Module());
+ fail("Expected injector creation failure");
+ } catch (CreationException expected) {
+ // both of our super class bindings cause errors
+ assertContains(expected.getMessage(),
+ "A binding to java.lang.Long was already configured",
+ "A binding to java.lang.Integer was already configured");
+ }
+ }
+
+ public void testProvidesMethodsDefinedInSuperClass() {
+ Injector injector = Guice.createInjector(new Sub1Module());
+ assertEquals(42, injector.getInstance(Integer.class).intValue());
+ assertEquals(42L, injector.getInstance(Long.class).longValue());
+ assertEquals(42D, injector.getInstance(Double.class).doubleValue());
+ }
+
+ private static class BaseModule extends AbstractModule {
+ @Override protected void configure() {}
+
+ @Provides Integer foo() {
+ return 42;
+ }
+
+ @Provides Long bar() {
+ return 42L;
+ }
+ }
+
+ private static class Sub1Module extends BaseModule {
+ @Provides Double baz() {
+ return 42D;
+ }
+ }
+
+ private static class Sub2Module extends BaseModule {
+ @Provides Float quux() {
+ return 42F;
+ }
+ }
+
+ /*if[AOP]*/
+ public void testShareFastClass() {
+ CallerInspecterModule module = new CallerInspecterModule();
+ Guice.createInjector(Stage.PRODUCTION, module);
+ assertEquals(module.fooCallerClass, module.barCallerClass);
+ assertTrue(module.fooCallerClass.contains("$$FastClassByGuice$$"));
+ }
+
+ private static class CallerInspecterModule extends AbstractModule {
+ // start them off as unequal
+ String barCallerClass = "not_set_bar";
+ String fooCallerClass = "not_set_foo";
+
+ @Override protected void configure() {}
+
+ @Provides @Singleton Integer foo() {
+ this.fooCallerClass = new
Exception().getStackTrace()[1].getClassName();
+ return 42;
+ }
+
+ @Provides @Singleton Long bar() {
+ this.barCallerClass = new
Exception().getStackTrace()[1].getClassName();
+ return 42L;
+ }
+ }
+
+ public void testShareFastClassWithSuperClass() {
+ CallerInspecterSubClassModule module = new
CallerInspecterSubClassModule();
+ Guice.createInjector(Stage.PRODUCTION, module);
+ assertEquals("Expected provider methods in the same class to share
fastclass classes",
+ module.fooCallerClass, module.barCallerClass);
+ assertFalse(
+ "Did not expect provider methods in the subclasses to share
fastclass classes "
+ + "with their parent classes",
+ module.bazCallerClass.equals(module.barCallerClass));
+ }
+
+
+ private static class CallerInspecterSubClassModule extends
CallerInspecterModule {
+ String bazCallerClass;
+
+ @Override protected void configure() {}
+
+ @Provides @Singleton Double baz() {
+ this.bazCallerClass = new
Exception().getStackTrace()[1].getClassName();
+ return 42D;
+ }
+ }
+ /*end[AOP]*/
+
+ // Test the behavior of provider methods when they are overridden
+ public void testOverrideProviderMethod() {
+ try {
+ Guice.createInjector(new SuperClassModule() {
+ @Override
+ @Provides
+ Double normalOverrideWithProvides() {
+ return 2D;
+ }
+ });
+ fail();
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(),
+ "A binding to java.lang.Double was already configured");
+ }
+
+ Injector injector = Guice.createInjector(Stage.PRODUCTION, new
SuperClassModule() {
+ @Override Long normalOverrideWithoutProvides() {
+ return 2L;
+ }
+ });
+ assertEquals(2L, injector.getInstance(Long.class).longValue());
+
+ injector = Guice.createInjector(Stage.PRODUCTION, new
SuperClassModule() {
+ @Override ImmutableSet<String>
covariantReturnOverrideWithoutProvides() {
+ return ImmutableSet.of("subset");
+ }
+ });
+ assertEquals(ImmutableSet.of("subset"),
+ injector.getInstance(new Key<Collection<String>>() {}));
+ // This is super weird since two keys get bound to the same method.
+ // TODO(sameb): make this throw an exception at configure() time!
+ injector = Guice.createInjector(Stage.PRODUCTION, new
SuperClassModule() {
+ @Override
+ @Provides
+ String covariantReturnOverrideWithProvides() {
+ return "sub";
+ }
+ });
+ assertEquals("sub", injector.getInstance(String.class));
+ assertEquals("sub", injector.getInstance(CharSequence.class));
+ }
+
+ private static class SuperClassModule extends AbstractModule {
+ @Override protected void configure() {}
+ @Provides Double normalOverrideWithProvides() {
+ return 1D;
+ }
+
+ @Provides Long normalOverrideWithoutProvides() {
+ return 1L;
+ }
+
+ @Provides CharSequence covariantReturnOverrideWithProvides() {
+ return "base";
+ }
+
+ @Provides Collection<String> covariantReturnOverrideWithoutProvides() {
+ return ImmutableList.of("baselist");
+ }
+ }
}
==============================================================================
Revision: 4a4d8257ed41
Author: Sam Berlin <[email protected]>
Date: Sat May 10 14:34:44 2014 UTC
Log: Block when transferring request scope instead of checking owners
& throwing exceptions. This lets users delete hacky spin loops.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=66541958
http://code.google.com/p/google-guice/source/detail?r=4a4d8257ed41
Modified:
/extensions/servlet/src/com/google/inject/servlet/GuiceFilter.java
/extensions/servlet/src/com/google/inject/servlet/ServletScopes.java
/extensions/servlet/test/com/google/inject/servlet/TransferRequestIntegrationTest.java
=======================================
--- /extensions/servlet/src/com/google/inject/servlet/GuiceFilter.java Mon
Mar 10 16:50:34 2014 UTC
+++ /extensions/servlet/src/com/google/inject/servlet/GuiceFilter.java Sat
May 10 14:34:44 2014 UTC
@@ -16,7 +16,6 @@
package com.google.inject.servlet;
-import com.google.common.base.Preconditions;
import com.google.common.base.Throwables;
import com.google.inject.Inject;
import com.google.inject.Key;
@@ -175,7 +174,6 @@
final HttpServletRequest originalRequest;
final HttpServletRequest request;
final HttpServletResponse response;
- volatile Thread owner;
Context(HttpServletRequest originalRequest, HttpServletRequest request,
HttpServletResponse response) {
@@ -196,18 +194,14 @@
return response;
}
- <T> T call(Callable<T> callable) throws Exception {
- Thread oldOwner = owner;
- Thread newOwner = Thread.currentThread();
- Preconditions.checkState(oldOwner == null || oldOwner == newOwner,
- "Trying to transfer request scope but original scope is still
active");
- owner = newOwner;
+ // Synchronized to prevent two threads from using the same request
+ // scope concurrently.
+ synchronized <T> T call(Callable<T> callable) throws Exception {
Context previous = localContext.get();
localContext.set(this);
try {
return callable.call();
} finally {
- owner = oldOwner;
localContext.set(previous);
}
}
=======================================
--- /extensions/servlet/src/com/google/inject/servlet/ServletScopes.java
Mon Mar 10 16:50:34 2014 UTC
+++ /extensions/servlet/src/com/google/inject/servlet/ServletScopes.java
Sat May 10 14:34:44 2014 UTC
@@ -244,15 +244,10 @@
* where you can detach the request processing thread while waiting for
data,
* and reattach to a different thread to finish processing at a later
time.
*
- * <p>Because {@code HttpServletRequest} objects are not typically
- * thread-safe, the callable returned by this method must not be run on a
- * different thread until the current request scope has terminated. In
other
- * words, do not use this method to propagate the current request scope
to
- * worker threads that may run concurrently with the current thread.
- *
- * <p>The returned callable will throw a {@link ScopingException} when
called
- * if the request scope being transferred is still active on a different
- * thread.
+ * <p>Because request-scoped objects are not typically thread-safe, the
+ * callable returned by this method must not be run on a different thread
+ * until the current request scope has terminated. The returned callable
will
+ * block until the current thread has released the request scope.
*
* @param callable code to be executed in another thread, which depends
on
* the request scope.
@@ -366,20 +361,15 @@
private static class Context {
final Map<Key, Object> map = Maps.newHashMap();
- volatile Thread owner;
- <T> T call(Callable<T> callable) throws Exception {
- Thread oldOwner = owner;
- Thread newOwner = Thread.currentThread();
- checkScopingState(oldOwner == null || oldOwner == newOwner,
- "Trying to transfer request scope but original scope is still
active");
- owner = newOwner;
+ // Synchronized to prevent two threads from using the same request
+ // scope concurrently.
+ synchronized <T> T call(Callable<T> callable) throws Exception {
Context previous = requestScopeContext.get();
requestScopeContext.set(this);
try {
return callable.call();
} finally {
- owner = oldOwner;
requestScopeContext.set(previous);
}
}
=======================================
---
/extensions/servlet/test/com/google/inject/servlet/TransferRequestIntegrationTest.java
Sun May 27 17:39:27 2012 UTC
+++
/extensions/servlet/test/com/google/inject/servlet/TransferRequestIntegrationTest.java
Sat May 10 14:34:44 2014 UTC
@@ -27,10 +27,11 @@
import junit.framework.TestCase;
import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
// TODO: Add test for HTTP transferring.
/**
@@ -88,16 +89,16 @@
executor.shutdownNow();
}
- public void testTransferNonHttpRequest_concurrentUseFails() throws
Exception {
+ public void testTransferNonHttpRequest_concurrentUseBlocks() throws
Exception {
Callable<Boolean> callable = new Callable<Boolean>() {
@Override public Boolean call() throws Exception {
ExecutorService executor = Executors.newSingleThreadExecutor();
try {
Future<Boolean> future =
executor.submit(ServletScopes.transferRequest(FALSE_CALLABLE));
try {
- return future.get();
- } catch (ExecutionException e) {
- return e.getCause() instanceof IllegalStateException;
+ return future.get(100, TimeUnit.MILLISECONDS);
+ } catch (TimeoutException e) {
+ return true;
}
} finally {
executor.shutdownNow();
--
You received this message because you are subscribed to the Google Groups
"google-guice-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/google-guice-dev.
For more options, visit https://groups.google.com/d/optout.
