Skip to content

Commit

Permalink
Simplify the API between factories and construction contexts by movin…
Browse files Browse the repository at this point in the history
…g more logic into InternalContext.

Instead of having factories manage a `ConstructionContext` object and call some subset of methods on it, we simplify things into a number of abstract operations on `InternalContext`
 - `tryStartConstruction`: called when we are starting construction
      - This is amortized O(1) work and only allocates when we are resizing the internal hash tables.
 - `finishConstruction`: called when construction is complete, clears the table
      - This is amortized O(1) work due to the hash search and some datastructure maintenance
 - `finishConstructionAndSetReference`: called by `ConstructorInjector` to support member injectors calling back into it
 - `clearCurrentReference`: called by `ConstructorInjector` after member injection is complete.

This enables the following optimizations

* When `disableCircularProxies` is set can get away with a single `int[]` to track the set of currently constructing factories and turn `finishConstructionAndSetReference` into a no-op
* When circular proxies are enabled we can eliminate the `ConstructionContext` object and only allocate when we actually construct a proxy.
* We can 'statically' allocate each factory a unique non-zero integer (a `circularFactoryId`). This allows us to create a open-addressed hashtable with integer keys that reduces allocation, hashing and GC overhead.
  - this does limit us to 2^32-1 circular factories before we get collisions, but that should be enough! pretty sure that other things will fail first.
* The new `InternalContext` protocol enables some simplifications in the factories which allows to delete an implementation

Previously we would insert `ConstructionContext` objects into the hash table and reuse them if the same factory was used multiple times.  This was generally rare, and now we instead remove keys from the table after construction.  In theory this means we always perform 2 hash table lookups per construction where the prior approach might only do one.  This is true but in general all lookups missed so the prior approach also required 2 lookups (`get` and `put`).

This is an optimization all on its own, but the new simpler protocol will enable more optimizations in a followup.

PiperOrigin-RevId: 680304599
  • Loading branch information
lukesandberg authored and Guice Team committed Jan 9, 2025
1 parent dabffd4 commit c1c50ca
Show file tree
Hide file tree
Showing 22 changed files with 978 additions and 344 deletions.
3 changes: 2 additions & 1 deletion core/src/com/google/inject/internal/BindingProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ public Boolean visit(ProviderInstanceBinding<? extends T> binding) {
new InternalFactoryToInitializableAdapter<T>(
initializable,
source,
injector.provisionListenerStore.get((ProviderInstanceBinding<T>) binding));
injector.provisionListenerStore.get((ProviderInstanceBinding<T>) binding),
injector.circularFactoryIdFactory.next());
InternalFactory<? extends T> scopedFactory =
Scoping.scope(key, injector, factory, source, scoping);
putBinding(
Expand Down
16 changes: 1 addition & 15 deletions core/src/com/google/inject/internal/BoundProviderFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.inject.Key;
import com.google.inject.internal.InjectorImpl.JitLimitation;
import com.google.inject.spi.Dependency;
import jakarta.inject.Provider;

/** Delegates to a custom factory which is also bound in the injector. */
final class BoundProviderFactory<T> extends ProviderInternalFactory<T> implements CreationListener {
Expand All @@ -34,7 +33,7 @@ final class BoundProviderFactory<T> extends ProviderInternalFactory<T> implement
Key<? extends jakarta.inject.Provider<? extends T>> providerKey,
Object source,
ProvisionListenerStackCallback<T> provisionCallback) {
super(source);
super(source, injector.circularFactoryIdFactory.next());
this.provisionCallback = provisionCallback;
this.injector = injector;
this.providerKey = providerKey;
Expand Down Expand Up @@ -62,19 +61,6 @@ public T get(InternalContext context, Dependency<?> dependency, boolean linked)
}
}

@Override
protected T provision(
Provider<? extends T> provider,
Dependency<?> dependency,
ConstructionContext<T> constructionContext)
throws InternalProvisionException {
try {
return super.provision(provider, dependency, constructionContext);
} catch (RuntimeException userException) {
throw InternalProvisionException.errorInProvider(userException);
}
}

@Override
public String toString() {
return providerKey.toString();
Expand Down
91 changes: 0 additions & 91 deletions core/src/com/google/inject/internal/ConstructionContext.java

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,12 @@ public ConstructorBindingImpl(
new DefaultConstructionProxyFactory<T>(constructorInjectionPoint).create();
this.constructorInjectionPoint = constructorInjectionPoint;
factory.constructorInjector =
new ConstructorInjector<T>(injectionPoints, constructionProxy, null, null);
new ConstructorInjector<T>(
injectionPoints,
constructionProxy,
null,
null,
/* circularFactoryId= */ InternalContext.CircularFactoryIdFactory.INVALID_ID);
}

/**
Expand Down
90 changes: 41 additions & 49 deletions core/src/com/google/inject/internal/ConstructorInjector.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,21 @@ final class ConstructorInjector<T> {
private final ImmutableSet<InjectionPoint> injectableMembers;
private final SingleParameterInjector<?>[] parameterInjectors;
private final ConstructionProxy<T> constructionProxy;
private final MembersInjectorImpl<T> membersInjector;
@Nullable private final MembersInjectorImpl<T> membersInjector;
private final int circularFactoryId;

ConstructorInjector(
Set<InjectionPoint> injectableMembers,
ConstructionProxy<T> constructionProxy,
SingleParameterInjector<?>[] parameterInjectors,
MembersInjectorImpl<T> membersInjector) {
@Nullable MembersInjectorImpl<T> membersInjector,
int circularFactoryId) {
this.injectableMembers = ImmutableSet.copyOf(injectableMembers);
this.constructionProxy = constructionProxy;
this.parameterInjectors = parameterInjectors;
this.membersInjector = membersInjector;
this.membersInjector =
membersInjector == null || membersInjector.isEmpty() ? null : membersInjector;
this.circularFactoryId = circularFactoryId;
}

public ImmutableSet<InjectionPoint> getInjectableMembers() {
Expand All @@ -65,72 +69,60 @@ Object construct(
Dependency<?> dependency,
@Nullable ProvisionListenerStackCallback<T> provisionCallback)
throws InternalProvisionException {
final ConstructionContext<T> constructionContext = context.getConstructionContext(this);
// We have a circular reference between constructors. Return a proxy.
if (constructionContext.isConstructing()) {
// TODO (user): if we can't proxy this object, can we proxy the other object?
return constructionContext.createProxy(
context.getInjectorOptions(), dependency.getKey().getTypeLiteral().getRawType());
@SuppressWarnings("unchecked")
T result = (T) context.tryStartConstruction(circularFactoryId, dependency);
if (result != null) {
// We have a circular reference between bindings. Return a proxy.
return result;
}

// If we're re-entering this factory while injecting fields or methods,
// return the same instance. This prevents infinite loops.
T t = constructionContext.getCurrentReference();
if (t != null) {
if (context.getInjectorOptions().disableCircularProxies) {
throw InternalProvisionException.circularDependenciesDisabled(
dependency.getKey().getTypeLiteral().getRawType());
}
return t;
}

constructionContext.startConstruction();
try {
// Optimization: Don't go through the callback stack if we have no listeners.
if (provisionCallback == null) {
return provision(context, constructionContext);
} else {
return provisionCallback.provision(
context,
new ProvisionCallback<T>() {
@Override
public T call() throws InternalProvisionException {
return provision(context, constructionContext);
}
});
}
} finally {
constructionContext.finishConstruction();
// Optimization: Don't go through the callback stack if we have no listeners.
if (provisionCallback == null) {
return provision(context);
} else {
// NOTE: `provision` always calls the callback, even if provision listeners
// throw exceptions.
return provisionCallback.provision(
context,
new ProvisionCallback<T>() {
@Override
public T call() throws InternalProvisionException {
return provision(context);
}
});
}
}

/** Provisions a new T. */
private T provision(InternalContext context, ConstructionContext<T> constructionContext)
throws InternalProvisionException {
private T provision(InternalContext context) throws InternalProvisionException {
MembersInjectorImpl<T> localMembersInjector = membersInjector;
try {
T t;
T t = null;
try {
Object[] parameters = SingleParameterInjector.getAll(context, parameterInjectors);
t = constructionProxy.newInstance(parameters);
constructionContext.setProxyDelegates(t);
} finally {
constructionContext.finishConstruction();
if (localMembersInjector == null) {
context.finishConstruction(circularFactoryId, t);
} else {
context.finishConstructionAndSetReference(circularFactoryId, t);
}
}

// Store reference. If an injector re-enters this factory, they'll get the same reference.
constructionContext.setCurrentReference(t);

MembersInjectorImpl<T> localMembersInjector = membersInjector;
localMembersInjector.injectMembers(t, context, false);
localMembersInjector.notifyListeners(t);
if (localMembersInjector != null) {
localMembersInjector.injectMembers(t, context, /* toolableOnly= */ false);
localMembersInjector.notifyListeners(t);
}

return t;
} catch (InvocationTargetException userException) {
Throwable cause = userException.getCause() != null ? userException.getCause() : userException;
throw InternalProvisionException.errorInjectingConstructor(cause)
.addSource(constructionProxy.getInjectionPoint());
} finally {
constructionContext.removeCurrentReference();
if (localMembersInjector != null) {
context.clearCurrentReference(circularFactoryId);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ private <T> ConstructorInjector<T> createConstructor(InjectionPoint injectionPoi
membersInjector.getInjectionPoints(),
factory.create(),
constructorParameterInjectors,
membersInjector);
membersInjector,
injector.circularFactoryIdFactory.next());
}
}
5 changes: 2 additions & 3 deletions core/src/com/google/inject/internal/FactoryProxy.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,10 @@ public void notify(final Errors errors) {
@Override
public T get(InternalContext context, Dependency<?> dependency, boolean linked)
throws InternalProvisionException {
Key<? extends T> localTargetKey = targetKey;
try {
return targetFactory.get(context, dependency, true);
return targetFactory.get(context, dependency, /* linked= */ true);
} catch (InternalProvisionException ipe) {
throw ipe.addSource(localTargetKey);
throw ipe.addSource(targetKey);
}
}

Expand Down
16 changes: 10 additions & 6 deletions core/src/com/google/inject/internal/InjectorImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ enum JitLimitation {
private final InjectorJitBindingData jitBindingData;
final InjectorImpl parent;
final InjectorOptions options;
final InternalContext.CircularFactoryIdFactory circularFactoryIdFactory;

Lookups lookups = new DeferredLookups(this);

Expand All @@ -136,11 +137,13 @@ enum JitLimitation {

if (parent != null) {
localContext = parent.localContext;
circularFactoryIdFactory = parent.circularFactoryIdFactory;
} else {
// No ThreadLocal.initialValue(), as that would cause classloader leaks. See
// https://github.com/google/guice/issues/288#issuecomment-48216933,
// https://github.com/google/guice/issues/288#issuecomment-48216944
localContext = new ThreadLocal<>();
circularFactoryIdFactory = new InternalContext.CircularFactoryIdFactory();
}
}

Expand Down Expand Up @@ -192,7 +195,7 @@ public <T> BindingImpl<T> getExistingBinding(Key<T> key) {
if (isProvider(key)) {
try {
// This is safe because isProvider above ensures that T is a Provider<?>
@SuppressWarnings({"unchecked", "cast"})
@SuppressWarnings("unchecked")
Key<?> providedKey = (Key<?>) getProvidedKey((Key) key, new Errors());
if (getExistingBinding(providedKey) != null) {
return getBinding(key);
Expand Down Expand Up @@ -683,7 +686,7 @@ private void removeFailedJitBinding(Binding<?> binding, InjectionPoint ip) {
/** Safely gets the dependencies of possibly not initialized bindings. */
private Set<Dependency<?>> getInternalDependencies(BindingImpl<?> binding) {
if (binding instanceof ConstructorBindingImpl) {
return ((ConstructorBindingImpl) binding).getInternalDependencies();
return ((ConstructorBindingImpl<?>) binding).getInternalDependencies();
} else if (binding instanceof HasDependencies) {
return ((HasDependencies) binding).getDependencies();
} else {
Expand Down Expand Up @@ -807,7 +810,8 @@ <T> BindingImpl<T> createProvidedByBinding(
@SuppressWarnings("unchecked")
Key<? extends Provider<T>> providerKey = (Key<? extends Provider<T>>) Key.get(providerType);
ProvidedByInternalFactory<T> internalFactory =
new ProvidedByInternalFactory<T>(rawType, providerType, providerKey);
new ProvidedByInternalFactory<T>(
rawType, providerType, providerKey, circularFactoryIdFactory.next());
Object source = rawType;
BindingImpl<T> binding =
LinkedProviderBindingImpl.createWithInitializer(
Expand Down Expand Up @@ -937,7 +941,7 @@ private <T> BindingImpl<T> createJustInTimeBinding(
if (isProvider(key)) {
// These casts are safe. We know T extends Provider<X> and that given Key<Provider<X>>,
// createSyntheticProviderBinding() will return BindingImpl<Provider<X>>.
@SuppressWarnings({"unchecked", "cast"})
@SuppressWarnings("unchecked")
BindingImpl<T> binding = (BindingImpl<T>) createSyntheticProviderBinding((Key) key, errors);
return binding;
}
Expand All @@ -946,7 +950,7 @@ private <T> BindingImpl<T> createJustInTimeBinding(
if (isMembersInjector(key)) {
// These casts are safe. T extends MembersInjector<X> and that given Key<MembersInjector<X>>,
// createMembersInjectorBinding() will return BindingImpl<MembersInjector<X>>.
@SuppressWarnings({"unchecked", "cast"})
@SuppressWarnings("unchecked")
BindingImpl<T> binding = (BindingImpl<T>) createMembersInjectorBinding((Key) key, errors);
return binding;
}
Expand Down Expand Up @@ -1228,7 +1232,7 @@ InternalContext enterContext() {
}
InternalContext ctx = (InternalContext) reference[0];
if (ctx == null) {
reference[0] = ctx = new InternalContext(options, reference);
reference[0] = ctx = InternalContext.create(options.disableCircularProxies, reference);
} else {
ctx.enter();
}
Expand Down
Loading

0 comments on commit c1c50ca

Please sign in to comment.