Skip to content

Commit

Permalink
Aggregate the Guice injector's JIT binding data into an InjectorJitBi…
Browse files Browse the repository at this point in the history
…ndingData object.

In particular:
- Move jitBindings and failedJitBindings out of InjectorImpl and into a new InjectorJitBindingData class, which is constructed in parallel with State in the InjectorShell.
- Move blacklistedKeys (and associated methods) out of InheritingState and into InjectorJitBindingData.
- Rename blacklistedKeys methods: blacklist -> banKey; isBlacklisted -> isBannedKey; getSourcesForBlacklistedKey -> getSourcesForBannedKey
- Replace parent().blacklist() with banKeyInParent() since it's the only reason the parent jit binding data is ever accessed.

This should be a refactor only: no change in behavior is expected from this CL.

This is Step 1 of this design doc: https://docs.google.com/document/d/1az6yShBLpN9DKr1uvFlMUQEqj-2eXl0frckmVWDEdkg/edit

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=320495996
  • Loading branch information
ad-fu authored and kevinb9n committed Jul 10, 2020
1 parent 85e30be commit 3d98f86
Show file tree
Hide file tree
Showing 9 changed files with 140 additions and 97 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ protected void putBinding(BindingImpl<?> binding) {
}

// prevent the parent from creating a JIT binding for this key
injector.state.parent().blacklist(key, injector.state, binding.getSource());
injector.getJitBindingData().banKeyInParent(key, injector.state, binding.getSource());
injector.state.putBinding(key, binding);
}

Expand Down
18 changes: 0 additions & 18 deletions core/src/com/google/inject/internal/InheritingState.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,11 @@ final class InheritingState implements State {
private final List<TypeListenerBinding> typeListenerBindings = Lists.newArrayList();
private final List<ProvisionListenerBinding> provisionListenerBindings = Lists.newArrayList();
private final List<ModuleAnnotatedMethodScannerBinding> scannerBindings = Lists.newArrayList();
private final WeakKeySet blacklistedKeys;
private final Object lock;

InheritingState(State parent) {
this.parent = checkNotNull(parent, "parent");
this.lock = (parent == State.NONE) ? this : parent.lock();
this.blacklistedKeys = new WeakKeySet(lock);
}

@Override
Expand Down Expand Up @@ -253,22 +251,6 @@ public List<ModuleAnnotatedMethodScannerBinding> getScannerBindingsThisLevel() {
return scannerBindings;
}

@Override
public void blacklist(Key<?> key, State state, Object source) {
parent.blacklist(key, state, source);
blacklistedKeys.add(key, state, source);
}

@Override
public boolean isBlacklisted(Key<?> key) {
return blacklistedKeys.contains(key);
}

@Override
public Set<Object> getSourcesForBlacklistedKey(Key<?> key) {
return blacklistedKeys.getSources(key);
}

@Override
public Object lock() {
return lock;
Expand Down
67 changes: 35 additions & 32 deletions core/src/com/google/inject/internal/InjectorImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimaps;
import com.google.common.collect.Sets;
import com.google.inject.Binder;
Expand Down Expand Up @@ -116,26 +115,24 @@ enum JitLimitation {
}

final State state;
private final InjectorJitBindingData jitBindingData;
final InjectorImpl parent;
final ListMultimap<TypeLiteral<?>, Binding<?>> bindingsMultimap = ArrayListMultimap.create();
final InjectorOptions options;

/** Just-in-time binding cache. Guarded by state.lock() */
final Map<Key<?>, BindingImpl<?>> jitBindings = Maps.newHashMap();
/**
* Cache of Keys that we were unable to create JIT bindings for, so we don't keep trying. Also
* guarded by state.lock().
*/
final Set<Key<?>> failedJitBindings = Sets.newHashSet();

Lookups lookups = new DeferredLookups(this);

/** The set of types passed to {@link #getMembersInjector} and {@link #injectMembers}. */
final Set<TypeLiteral<?>> userRequestedMembersInjectorTypes = Sets.newConcurrentHashSet();

InjectorImpl(InjectorImpl parent, State state, InjectorOptions injectorOptions) {
InjectorImpl(
InjectorImpl parent,
State state,
InjectorJitBindingData jitBindingData,
InjectorOptions injectorOptions) {
this.parent = parent;
this.state = state;
this.jitBindingData = jitBindingData;
this.options = injectorOptions;

if (parent != null) {
Expand Down Expand Up @@ -186,7 +183,8 @@ public <T> BindingImpl<T> getExistingBinding(Key<T> key) {
// See if any jit bindings have been created for this key.
for (InjectorImpl injector = this; injector != null; injector = injector.parent) {
@SuppressWarnings("unchecked")
BindingImpl<T> jitBinding = (BindingImpl<T>) injector.jitBindings.get(key);
BindingImpl<T> jitBinding =
(BindingImpl<T>) injector.jitBindingData.getJitBindings().get(key);
if (jitBinding != null) {
return jitBinding;
}
Expand Down Expand Up @@ -250,6 +248,10 @@ public Injector createChildInjector(Module... modules) {
return createChildInjector(ImmutableList.copyOf(modules));
}

InjectorJitBindingData getJitBindingData() {
return jitBindingData;
}

/**
* Returns a just-in-time binding for {@code key}, creating it if necessary.
*
Expand All @@ -263,7 +265,7 @@ private <T> BindingImpl<T> getJustInTimeBinding(Key<T> key, Errors errors, JitLi
// first try to find a JIT binding that we've already created
for (InjectorImpl injector = this; injector != null; injector = injector.parent) {
@SuppressWarnings("unchecked") // we only store bindings that match their key
BindingImpl<T> binding = (BindingImpl<T>) injector.jitBindings.get(key);
BindingImpl<T> binding = (BindingImpl<T>) injector.jitBindingData.getJitBindings().get(key);

if (binding != null) {
// If we found a JIT binding and we don't allow them,
Expand All @@ -285,15 +287,15 @@ private <T> BindingImpl<T> getJustInTimeBinding(Key<T> key, Errors errors, JitLi
// entry in jitBindings (during cleanup), and we may be trying
// to create it again (in the case of a recursive JIT binding).
// We need both of these guards for different reasons
// failedJitBindings.contains: We want to continue processing if we've never
// getFailedJitBindings.contains: We want to continue processing if we've never
// failed before, so that our initial error message contains
// as much useful information as possible about what errors exist.
// errors.hasErrors: If we haven't already failed, then it's OK to
// continue processing, to make sure the ultimate error message
// is the correct one.
// See: ImplicitBindingsTest#testRecursiveJitBindingsCleanupCorrectly
// for where this guard compes into play.
if (failedJitBindings.contains(key) && errors.hasErrors()) {
// for where this guard comes into play.
if (jitBindingData.getFailedJitBindings().contains(key) && errors.hasErrors()) {
throw errors.toException();
}
return createJustInTimeBindingRecursive(key, errors, options.jitDisabled, jitType);
Expand Down Expand Up @@ -598,7 +600,7 @@ <T> void initializeJitBinding(BindingImpl<T> binding, Errors errors) throws Erro
// Note: We don't need to synchronize on state.lock() during injector creation.
if (binding instanceof DelayedInitialize) {
Key<T> key = binding.getKey();
jitBindings.put(key, binding);
jitBindingData.getJitBindings().put(key, binding);
boolean successful = false;
DelayedInitialize delayed = (DelayedInitialize) binding;
try {
Expand Down Expand Up @@ -629,7 +631,7 @@ private boolean cleanup(BindingImpl<?> binding, Set<Key> encountered) {
Key<?> depKey = dep.getKey();
InjectionPoint ip = dep.getInjectionPoint();
if (encountered.add(depKey)) { // only check if we haven't looked at this key yet
BindingImpl depBinding = jitBindings.get(depKey);
BindingImpl<?> depBinding = jitBindingData.getJitBindings().get(depKey);
if (depBinding != null) { // if the binding still exists, validate
boolean failed = cleanup(depBinding, encountered); // if children fail, we fail
if (depBinding instanceof ConstructorBindingImpl) {
Expand All @@ -655,8 +657,8 @@ private boolean cleanup(BindingImpl<?> binding, Set<Key> encountered) {

/** Cleans up any state that may have been cached when constructing the JIT binding. */
private void removeFailedJitBinding(Binding<?> binding, InjectionPoint ip) {
failedJitBindings.add(binding.getKey());
jitBindings.remove(binding.getKey());
jitBindingData.getFailedJitBindings().add(binding.getKey());
jitBindingData.getJitBindings().remove(binding.getKey());
membersInjectorStore.remove(binding.getKey().getTypeLiteral());
provisionListenerStore.remove(binding);
if (ip != null) {
Expand Down Expand Up @@ -846,22 +848,23 @@ private <T> BindingImpl<T> createJustInTimeBindingRecursive(
jitDisabled,
parent.options.jitDisabled ? JitLimitation.NO_JIT : jitType);
} catch (ErrorsException ignored) {
// TODO(b/160910914): Why are ErrorsExceptions ignored?
}
}

// Retrieve the sources before checking for blacklisting to guard against sources becoming null
// due to a full GC happening after calling state.isBlacklisted and
// state.getSourcesForBlacklistedKey.
// Retrieve the sources before checking for banned key to guard against sources becoming null
// due to a full GC happening after calling jitBindingData.isBanned and
// state.getSourcesForBannedKey.
// TODO(user): Consolidate these two APIs.
Set<Object> sources = state.getSourcesForBlacklistedKey(key);
if (state.isBlacklisted(key)) {
Set<Object> sources = jitBindingData.getSourcesForBannedKey(key);
if (jitBindingData.isBannedKey(key)) {
throw errors.childBindingAlreadySet(key, sources).toException();
}

key = MoreTypes.canonicalizeKey(key); // before storing the key long-term, canonicalize it.
BindingImpl<T> binding = createJustInTimeBinding(key, errors, jitDisabled, jitType);
state.parent().blacklist(key, state, binding.getSource());
jitBindings.put(key, binding);
jitBindingData.banKeyInParent(key, state, binding.getSource());
jitBindingData.getJitBindings().put(key, binding);
return binding;
}

Expand All @@ -884,12 +887,12 @@ private <T> BindingImpl<T> createJustInTimeBinding(
throws ErrorsException {
int numErrorsBefore = errors.size();

// Retrieve the sources before checking for blacklisting to guard against sources becoming null
// due to a full GC happening after calling state.isBlacklisted and
// state.getSourcesForBlacklistedKey.
// Retrieve the sources before checking for a banned key to guard against sources becoming null
// due to a full GC happening after calling jitBindingData.isBanned and
// jitBindingData.getSourcesForBannedKey.
// TODO(user): Consolidate these two APIs.
Set<Object> sources = state.getSourcesForBlacklistedKey(key);
if (state.isBlacklisted(key)) {
Set<Object> sources = jitBindingData.getSourcesForBannedKey(key);
if (jitBindingData.isBannedKey(key)) {
throw errors.childBindingAlreadySet(key, sources).toException();
}

Expand Down Expand Up @@ -958,7 +961,7 @@ public Map<Key<?>, Binding<?>> getAllBindings() {
synchronized (state.lock()) {
return new ImmutableMap.Builder<Key<?>, Binding<?>>()
.putAll(state.getExplicitBindingsThisLevel())
.putAll(jitBindings)
.putAll(jitBindingData.getJitBindings())
.build();
}
}
Expand Down
87 changes: 87 additions & 0 deletions core/src/com/google/inject/internal/InjectorJitBindingData.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package com.google.inject.internal;

import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.inject.Key;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

/** A container for just-in-time (JIT) binding data corresponding to an Injector. */
final class InjectorJitBindingData {
// TODO(b/159459925): the State object is currently used as the lock for accessing all the JIT
// binding data fields. Migrate to using InjectorJitBindingData as a lock for these accesses
// instead.
// TODO(b/159459925): Hide direct access to internal jitBindings and failedJitBindings fields once
// locks are managed by this class
/** Just-in-time binding cache. Guarded by state.lock() */
private final Map<Key<?>, BindingImpl<?>> jitBindings = Maps.newHashMap();
/**
* Cache of Keys that we were unable to create JIT bindings for, so we don't keep trying. Also
* guarded by state.lock().
*/
private final Set<Key<?>> failedJitBindings = Sets.newHashSet();

// The set of JIT binding keys that are banned for this particular injector, because a binding
// already exists in a child injector.
private final WeakKeySet bannedKeys;

// The InjectorJitBindingData corresponding to the Injector's parent, if it exists.
private final Optional<InjectorJitBindingData> parent;

InjectorJitBindingData(Optional<InjectorJitBindingData> parent, State state) {
this.parent = parent;
this.bannedKeys = new WeakKeySet(state.lock());
}

/**
* Returns a mutable map containing the JIT bindings for the injector. Accesses to this need to be
* guarded by state.lock().
*/
Map<Key<?>, BindingImpl<?>> getJitBindings() {
return jitBindings;
}

/**
* Returns a mutable set containing the failed JIT bindings for the injector. Accesses to this
* need to be guarded by state.lock().
*/
Set<Key<?>> getFailedJitBindings() {
return failedJitBindings;
}

/**
* Forbids the corresponding injector and its ancestors from creating a binding to {@code key}.
* Child injectors ban their bound keys on their parent injectors to prevent just-in-time bindings
* on the parent injector that would conflict, and pass along their State to control the banned
* key's lifetime.
*/
void banKey(Key<?> key, State state, Object source) {
banKeyInParent(key, state, source);
bannedKeys.add(key, state, source);
}

/**
* Similar to {@link #banKey(Key, State, Object)} but we only begin banning the binding at the
* parent level. This is used to prevent JIT bindings in the parent injector from overriding
* explicit bindings declared in a child injector.
*/
void banKeyInParent(Key<?> key, State state, Object source) {
if (parent.isPresent()) {
parent.get().banKey(key, state, source);
}
}

/**
* Returns true if {@code key} is forbidden from being bound in the injector corresponding to this
* data object. This indicates that one of the injector's children has bound the key.
*/
boolean isBannedKey(Key<?> key) {
return bannedKeys.contains(key);
}

/** Returns the source of a banned key. */
Set<Object> getSourcesForBannedKey(Key<?> key) {
return bannedKeys.getSources(key);
}
}
13 changes: 11 additions & 2 deletions core/src/com/google/inject/internal/InjectorShell.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import com.google.inject.spi.ProvisionListenerBinding;
import com.google.inject.spi.TypeListenerBinding;
import java.util.List;
import java.util.Optional;
import java.util.logging.Logger;

/**
Expand Down Expand Up @@ -72,8 +73,9 @@ static class Builder {
private final List<Element> elements = Lists.newArrayList();
private final List<Module> modules = Lists.newArrayList();

/** lazily constructed */
// lazily constructed fields
private State state;
private InjectorJitBindingData jitBindingData;

private InjectorImpl parent;
private InjectorOptions options;
Expand All @@ -90,6 +92,8 @@ Builder stage(Stage stage) {
Builder parent(InjectorImpl parent) {
this.parent = parent;
this.state = new InheritingState(parent.state);
this.jitBindingData =
new InjectorJitBindingData(Optional.of(parent.getJitBindingData()), state);
this.options = parent.options;
this.stage = options.stage;
return this;
Expand Down Expand Up @@ -154,7 +158,7 @@ List<InjectorShell> build(
optionsProcessor.process(null, elements);
options = optionsProcessor.getOptions(stage, options);

InjectorImpl injector = new InjectorImpl(parent, state, options);
InjectorImpl injector = new InjectorImpl(parent, state, jitBindingData, options);
if (privateElements != null) {
privateElements.initInjector(injector);
}
Expand Down Expand Up @@ -216,9 +220,14 @@ List<InjectorShell> build(
return injectorShells;
}

/**
* Lazily initializes state and jitBindingData, if they were not already initialized with a
* parent injector by {@link #parent(InjectorImpl)}.
*/
private State getState() {
if (state == null) {
state = new InheritingState(State.NONE);
jitBindingData = new InjectorJitBindingData(Optional.empty(), state);
}
return state;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ void loadEagerSingletons(InjectorImpl injector, Stage stage, final Errors errors
candidateBindings.addAll(bindingsAtThisLevel);
synchronized (injector.state.lock()) {
// jit bindings must be accessed while holding the lock.
candidateBindings.addAll(injector.jitBindings.values());
candidateBindings.addAll(injector.getJitBindingData().getJitBindings().values());
}
InternalContext context = injector.enterContext();
try {
Expand Down
Loading

0 comments on commit 3d98f86

Please sign in to comment.