Skip to content

Commit

Permalink
Improve the performance of isCircularProxy
Browse files Browse the repository at this point in the history
The map implementation used by `MapMaker` is fairly old and requires 2 levels of indirection (segments->table).  Switching to `ConcurrentHashMap` would be better but this will still be redundant with logic inside of `Proxy` so instead we just use that.  Based on some simple benchmarks this is about 5X as fast, and should save a bit memory.

A number of alternatives were considered

* adding a marker interface to the list of proxy interfaces
  This is a completely ideal solution but fails due to our need to support proxying interfaces from all `ClassLoaders` and it isn't possible to proxy interfaces that cannot mutually 'see' each other.  So we would need to 'inject' our interface into the Bootstrap classloader which seems impossible and risky, or create special 'child' classloaders that can bridge it, but this triggers issues with proxying package-private interfaces... sigh.
* using a `ClassValue`, bootstrapping the value is tricky and while this is faster than the status quo, it would allocate an entry for every class we queried which could add up in a large application, and the approach in this change is faster.
* using `Proxy.isProxyInstance` this works but is slower than an `instanceof` query.

PiperOrigin-RevId: 721984180
  • Loading branch information
lukesandberg authored and Guice Team committed Feb 1, 2025
1 parent 9e4df6f commit 41e8c06
Showing 1 changed file with 14 additions and 10 deletions.
24 changes: 14 additions & 10 deletions core/src/com/google/inject/internal/BytecodeGen.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,13 @@
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.MapMaker;
import com.google.inject.internal.aop.ClassBuilding;
import java.lang.reflect.Constructor;
import java.lang.reflect.Executable;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.BitSet;
import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;

Expand All @@ -51,19 +49,23 @@
*/
public final class BytecodeGen {

private static final Map<Class<?>, Boolean> circularProxyTypeCache =
new MapMaker().weakKeys().makeMap();

/** Returns true if the given object is a circular proxy. */
public static boolean isCircularProxy(Object object) {
return object != null && circularProxyTypeCache.containsKey(object.getClass());
return object instanceof Proxy
&& Proxy.getInvocationHandler(object) instanceof DelegatingInvocationHandler;
}

/** Creates a new circular proxy for the given type. */
static <T> T newCircularProxy(Class<T> type, InvocationHandler handler) {
Object proxy = Proxy.newProxyInstance(type.getClassLoader(), new Class<?>[] {type}, handler);
circularProxyTypeCache.put(proxy.getClass(), Boolean.TRUE);
return type.cast(proxy);
static <T> T newCircularProxy(Class<T> type, DelegatingInvocationHandler handler) {
// Ideally we would add a marker interface to the list of interfaces to implement so that
// `isCircularProxy` could just use an instanceof test, but this does not work if the type
// in question is owned by a classloader that isn't ours or a descendant of ours.
// The other option is something like ClassValue, but it is difficult to bootstrap and would
// consume memory for every class passed to `isCircularProxy` which is much larger than the
// number of circular proxies.
@SuppressWarnings("unchecked") // This is guaranteed by the contract on newProxyInstance
T proxy = (T) Proxy.newProxyInstance(type.getClassLoader(), new Class<?>[] {type}, handler);
return proxy;
}

public static final String ENHANCER_BY_GUICE_MARKER = "$$EnhancerByGuice$$";
Expand Down Expand Up @@ -176,4 +178,6 @@ protected Function<String, BiFunction<Object, Object[], Object>> computeValue(Cl
return buildFastClass(hostClass);
}
};

private BytecodeGen() {}
}

0 comments on commit 41e8c06

Please sign in to comment.