Implement binding deduplication for multibinder & mapbinder in a diff…

…erent way. Instead of relying on Guice binding deduplication (and hacking up RealElement to break the annotation contract to do so, causing weirdness in WeakKeySet & forcing us to care about "rehashing keys"), we instead deduplicate within Multibinder. The downside of this is that toInstance or toProvider(instance) bindings that are deduplicated will remain in the object graph but effectively be unreachable. However, that's a downside I'm willing to live with to remove this hack. ------------- Created by MOE: http://code.google.com/p/moe-java MOE_MIGRATED_REVID=72570932
agent-mo · Aug 6, 2014 · c34e018 · c34e018
1 parent 22de684
commit c34e018
Show file tree

Hide file tree

Showing 26 changed files with 457 additions and 679 deletions.
diff --git a/core/src/com/google/inject/Key.java b/core/src/com/google/inject/Key.java
@@ -143,9 +143,7 @@ private Key(TypeLiteral<T> typeLiteral, AnnotationStrategy annotationStrategy) {
   }
 
   /**
-   * Computes the hash code for this key. This logic is duplicated in {@link
-   * com.google.inject.internal.RehashableKeys.Keys#needsRehashing}; if it is
-   * changed here, be sure to change it there also.
+   * Computes the hash code for this key.
    */
   private int computeHashCode() {
     return typeLiteral.hashCode() * 31 + annotationStrategy.hashCode();

diff --git a/core/src/com/google/inject/internal/BindingBuilder.java b/core/src/com/google/inject/internal/BindingBuilder.java
@@ -41,7 +41,7 @@
  * @author [email protected] (Jesse Wilson)
  */
 public class BindingBuilder<T> extends AbstractBindingBuilder<T>
-    implements AnnotatedBindingBuilder<T>, RehashableKeys {
+    implements AnnotatedBindingBuilder<T> {
 
   public BindingBuilder(Binder binder, List<Element> elements, Object source, Key<T> key) {
     super(binder, elements, source, key);
@@ -171,10 +171,6 @@ public <S extends T> ScopedBindingBuilder toConstructor(Constructor<S> construct
     return this;
   }
 
-  public void rehashKeys() {
-    setBinding(getBinding().withRehashedKeys());
-  }
-
   @Override public String toString() {
     return "BindingBuilder<" + getBinding().getKey().getTypeLiteral() + ">";
   }

diff --git a/core/src/com/google/inject/internal/BindingImpl.java b/core/src/com/google/inject/internal/BindingImpl.java
@@ -105,10 +105,6 @@ protected BindingImpl<T> withKey(Key<T> key) {
     throw new AssertionError();
   }
 
-  public BindingImpl<T> withRehashedKeys() {
-    throw new AssertionError();
-  }
-
   @Override public String toString() {
     return Objects.toStringHelper(Binding.class)
         .add("key", key)
@@ -120,4 +116,4 @@ public BindingImpl<T> withRehashedKeys() {
   public InjectorImpl getInjector() {
     return injector;
   }
-}
+}
diff --git a/core/src/com/google/inject/internal/ConstructorBindingImpl.java b/core/src/com/google/inject/internal/ConstructorBindingImpl.java
@@ -18,8 +18,6 @@
 
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.inject.internal.Annotations.findScopeAnnotation;
-import static com.google.inject.internal.RehashableKeys.Keys.needsRehashing;
-import static com.google.inject.internal.RehashableKeys.Keys.rehash;
 
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableSet;
@@ -213,14 +211,6 @@ public Set<Dependency<?>> getDependencies() {
         null, key, getSource(), factory, getScoping(), factory, constructorInjectionPoint);
   }
 
-  @Override public BindingImpl<T> withRehashedKeys() {
-    if (needsRehashing(getKey())) {
-      return withKey(rehash(getKey()));
-    } else {
-      return this;
-    }
-  }
-
   @SuppressWarnings("unchecked") // the raw constructor member and declaring type always agree
   public void applyTo(Binder binder) {
     InjectionPoint constructor = getConstructor();

diff --git a/core/src/com/google/inject/internal/InstanceBindingImpl.java b/core/src/com/google/inject/internal/InstanceBindingImpl.java
@@ -16,9 +16,6 @@
 
 package com.google.inject.internal;
 
-import static com.google.inject.internal.RehashableKeys.Keys.needsRehashing;
-import static com.google.inject.internal.RehashableKeys.Keys.rehash;
-
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableSet;
 import com.google.inject.Binder;
@@ -86,14 +83,6 @@ public BindingImpl<T> withKey(Key<T> key) {
     return new InstanceBindingImpl<T>(getSource(), key, getScoping(), injectionPoints, instance);
   }
 
-  public BindingImpl<T> withRehashedKeys() {
-    if (needsRehashing(getKey())) {
-      return withKey(rehash(getKey()));
-    } else {
-      return this;
-    }
-  }
-
   public void applyTo(Binder binder) {
     // instance bindings aren't scoped
     binder.withSource(getSource()).bind(getKey()).toInstance(instance);

diff --git a/core/src/com/google/inject/internal/LinkedBindingImpl.java b/core/src/com/google/inject/internal/LinkedBindingImpl.java
@@ -16,9 +16,6 @@
 
 package com.google.inject.internal;
 
-import static com.google.inject.internal.RehashableKeys.Keys.needsRehashing;
-import static com.google.inject.internal.RehashableKeys.Keys.rehash;
-
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableSet;
 import com.google.inject.Binder;
@@ -66,20 +63,6 @@ public BindingImpl<T> withKey(Key<T> key) {
     return new LinkedBindingImpl<T>(getSource(), key, getScoping(), targetKey);
   }
 
-  public BindingImpl<T> withRehashedKeys() {
-    boolean keyNeedsRehashing = needsRehashing(getKey());
-    boolean targetKeyNeedsRehashing = needsRehashing(targetKey);
-    if (keyNeedsRehashing || targetKeyNeedsRehashing) {
-      return new LinkedBindingImpl<T>(
-          getSource(),
-          keyNeedsRehashing ? rehash(getKey()) : getKey(),
-          getScoping(),
-          targetKeyNeedsRehashing ? rehash(targetKey) : targetKey);
-    } else {
-      return this;
-    }
-  }
-
   public void applyTo(Binder binder) {
     getScoping().applyTo(binder.withSource(getSource()).bind(getKey()).to(getLinkedKey()));
   }

diff --git a/core/src/com/google/inject/internal/LinkedProviderBindingImpl.java b/core/src/com/google/inject/internal/LinkedProviderBindingImpl.java
@@ -16,9 +16,6 @@
 
 package com.google.inject.internal;
 
-import static com.google.inject.internal.RehashableKeys.Keys.needsRehashing;
-import static com.google.inject.internal.RehashableKeys.Keys.rehash;
-
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableSet;
 import com.google.inject.Binder;
@@ -92,20 +89,6 @@ public BindingImpl<T> withKey(Key<T> key) {
     return new LinkedProviderBindingImpl<T>(getSource(), key, getScoping(), providerKey);
   }
 
-  public BindingImpl<T> withRehashedKeys() {
-    boolean keyNeedsRehashing = needsRehashing(getKey());
-    boolean providerKeyNeedsRehashing = needsRehashing(providerKey);
-    if (keyNeedsRehashing || providerKeyNeedsRehashing) {
-      return new LinkedProviderBindingImpl<T>(
-          getSource(),
-          keyNeedsRehashing ? rehash(getKey()) : getKey(),
-          getScoping(),
-          providerKeyNeedsRehashing ? rehash(providerKey) : providerKey);
-    } else {
-      return this;
-    }
-  }
-
   public void applyTo(Binder binder) {
     getScoping().applyTo(binder.withSource(getSource())
         .bind(getKey()).toProvider(getProviderKey()));

diff --git a/core/src/com/google/inject/internal/ProviderInstanceBindingImpl.java b/core/src/com/google/inject/internal/ProviderInstanceBindingImpl.java
@@ -16,9 +16,6 @@
 
 package com.google.inject.internal;
 
-import static com.google.inject.internal.RehashableKeys.Keys.needsRehashing;
-import static com.google.inject.internal.RehashableKeys.Keys.rehash;
-
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableSet;
 import com.google.inject.Binder;
@@ -94,14 +91,6 @@ public BindingImpl<T> withKey(Key<T> key) {
         getSource(), key, getScoping(), injectionPoints, providerInstance);
   }
 
-  public BindingImpl<T> withRehashedKeys() {
-    if (needsRehashing(getKey())) {
-      return withKey(rehash(getKey()));
-    } else {
-      return this;
-    }
-  }
-
   public void applyTo(Binder binder) {
     getScoping().applyTo(
         binder.withSource(getSource()).bind(getKey()).toProvider(getUserSuppliedProvider()));

diff --git a/core/src/com/google/inject/internal/RehashableKeys.java b/core/src/com/google/inject/internal/RehashableKeys.java
diff --git a/core/src/com/google/inject/internal/UntargettedBindingImpl.java b/core/src/com/google/inject/internal/UntargettedBindingImpl.java
@@ -16,9 +16,6 @@
 
 package com.google.inject.internal;
 
-import static com.google.inject.internal.RehashableKeys.Keys.needsRehashing;
-import static com.google.inject.internal.RehashableKeys.Keys.rehash;
-
 import com.google.common.base.Objects;
 import com.google.inject.Binder;
 import com.google.inject.Key;
@@ -52,14 +49,6 @@ public BindingImpl<T> withKey(Key<T> key) {
     return new UntargettedBindingImpl<T>(getSource(), key, getScoping());
   }
 
-  public BindingImpl<T> withRehashedKeys() {
-    if (needsRehashing(getKey())) {
-      return withKey(rehash(getKey()));
-    } else {
-      return this;
-    }
-  }
-
   public void applyTo(Binder binder) {
     getScoping().applyTo(binder.withSource(getSource()).bind(getKey()));
   }

diff --git a/core/src/com/google/inject/internal/WeakKeySet.java b/core/src/com/google/inject/internal/WeakKeySet.java
@@ -41,7 +41,7 @@
  */
 final class WeakKeySet {
 
-  private Map<BlacklistKey, Multiset<Object>> backingMap;
+  private Map<Key<?>, Multiset<Object>> backingMap;
 
   /**
    * This is already locked externally on add and getSources but we need it to handle clean up in
@@ -73,11 +73,11 @@ public void onRemoval(RemovalNotification<State, Set<KeyAndSource>> notification
   private void cleanUpForCollectedState(Set<KeyAndSource> keysAndSources) {
     synchronized (lock) {
       for (KeyAndSource keyAndSource : keysAndSources) {
-        Multiset<Object> set = backingMap.get(keyAndSource.blacklistKey);
+        Multiset<Object> set = backingMap.get(keyAndSource.key);
         if (set != null) {
           set.remove(keyAndSource.source);
           if (set.isEmpty()) {
-            backingMap.remove(keyAndSource.blacklistKey);
+            backingMap.remove(keyAndSource.key);
           }
         }
       }
@@ -97,11 +97,10 @@ public void add(Key<?> key, State state, Object source) {
     if (source instanceof Class || source == SourceProvider.UNKNOWN_SOURCE) {
       source = null;
     }
-    BlacklistKey blacklistKey = new BlacklistKey(key);
-    Multiset<Object> sources = backingMap.get(blacklistKey);
+    Multiset<Object> sources = backingMap.get(key);
     if (sources == null) {
       sources = LinkedHashMultiset.create();
-      backingMap.put(blacklistKey, sources);
+      backingMap.put(key, sources);
     }
     Object convertedSource = Errors.convert(source);
     sources.add(convertedSource);
@@ -112,33 +111,33 @@ public void add(Key<?> key, State state, Object source) {
       if (keyAndSources == null) {
         evictionCache.put(state, keyAndSources = Sets.newHashSet());
       }
-      keyAndSources.add(new KeyAndSource(blacklistKey, convertedSource));
+      keyAndSources.add(new KeyAndSource(key, convertedSource));
     }
   }
 
   public boolean contains(Key<?> key) {
     evictionCache.cleanUp();
-    return backingMap != null && backingMap.containsKey(new BlacklistKey(key));
+    return backingMap != null && backingMap.containsKey(key);
   }
 
   public Set<Object> getSources(Key<?> key) {
     evictionCache.cleanUp();
-    Multiset<Object> sources = (backingMap == null) ? null : backingMap.get(new BlacklistKey(key));
+    Multiset<Object> sources = (backingMap == null) ? null : backingMap.get(key);
     return (sources == null) ? null : sources.elementSet();
   }
 
   private static final class KeyAndSource {
-    final BlacklistKey blacklistKey;
+    final Key<?> key;
     final Object source;
 
-    KeyAndSource(BlacklistKey blacklistKey, Object source) {
-      this.blacklistKey = blacklistKey;
+    KeyAndSource(Key<?> key, Object source) {
+      this.key = key;
       this.source = source;
     }
 
     @Override
     public int hashCode() {
-      return Objects.hashCode(blacklistKey, source);
+      return Objects.hashCode(key, source);
     }
 
     @Override
@@ -152,42 +151,10 @@ public boolean equals(Object obj) {
       }
 
       KeyAndSource other = (KeyAndSource) obj;
-      return Objects.equal(blacklistKey, other.blacklistKey)
+      return Objects.equal(key, other.key)
           && Objects.equal(source, other.source);
     }
   }
-
-  /**
-   * The key for the Map is {@link Key} for most bindings, String for multibindings.
-   * <p>
-   * Reason being that multibinding Key's annotations hold a reference to their injector, implying
-   * we'd leak memory.
-   */
-  private static final class BlacklistKey {
-    final Object delegate;
-
-    BlacklistKey(Key<?> key) {
-      // HACK: See comment on BlacklistKey for more info. This is tested in MultibinderTest,
-      // MapBinderTest, and OptionalBinderTest in the multibinder test suite.
-      if (isMultiBinderKey(key)) {
-        delegate = key.toString();
-      } else {
-        delegate = key;
-      }
-    }
-
-    public boolean equals(Object obj) {
-      if (obj instanceof BlacklistKey) {
-        return delegate.equals(((BlacklistKey) obj).delegate);
-      } else {
-        return false;
-      }
-    }
-
-    public int hashCode() {
-      return delegate.hashCode();
-    }
-  }
 
   /**
    * Returns {@code true} if the {@link Key} represents a multibound element.