[CALCITE-2717] Use Interner instead of LoadingCache to cache traits to allow GC (Haisheng Yuan)
Even though canonicalMap's value is soft referenced, key is strong referenced, key and value are referencing the same object. So traits in the cache will never be garbage-collected, which may cause OOM if we have tons of different traits. This issue has been fixed by caching traits using Interner instead of LoadingCache. In addition, canonizeComposite is removed, since canonize can do the same work. Close apache/calcite#951 Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/de9a7164 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/de9a7164 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/de9a7164 Branch: refs/heads/master Commit: de9a7164b2d3e5a1637254fd0d54c7a4c886166e Parents: 4525840 Author: Haisheng Yuan <[email protected]> Authored: Fri Nov 30 14:25:20 2018 -0800 Committer: Julian Hyde <[email protected]> Committed: Fri Nov 30 20:29:37 2018 -0800 ---------------------------------------------------------------------- .../apache/calcite/plan/RelCompositeTrait.java | 24 ++------- .../org/apache/calcite/plan/RelTraitDef.java | 57 +++++--------------- 2 files changed, 15 insertions(+), 66 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/de9a7164/core/src/main/java/org/apache/calcite/plan/RelCompositeTrait.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/plan/RelCompositeTrait.java b/core/src/main/java/org/apache/calcite/plan/RelCompositeTrait.java index 892088e..e3555b8 100644 --- a/core/src/main/java/org/apache/calcite/plan/RelCompositeTrait.java +++ b/core/src/main/java/org/apache/calcite/plan/RelCompositeTrait.java @@ -67,7 +67,7 @@ class RelCompositeTrait<T extends RelMultipleTrait> implements RelTrait { } compositeTrait = new RelCompositeTrait<>(def, (T[]) traits); } - return def.canonizeComposite(compositeTrait); + return def.canonize(compositeTrait); } public RelTraitDef getTraitDef() { @@ -105,30 +105,12 @@ class RelCompositeTrait<T extends RelMultipleTrait> implements RelTrait { return ImmutableList.copyOf(traits); } - RelCompositeTrait<T> canonize(RelTraitDef<T> traitDef) { - T[] newTraits = null; - for (int i = 0; i < traits.length; i++) { - final T trait = traits[i]; - final T trait2 = traitDef.canonize(trait); - if (trait2 != trait) { - if (newTraits == null) { - newTraits = traits.clone(); - } - newTraits[i] = trait2; - } - } - if (newTraits == null) { - return this; - } - assert false; - // TODO: cache duplicate composites - return new RelCompositeTrait<>(traitDef, newTraits); - } - + /** Returns the {@code i}th trait. */ public T trait(int i) { return traits[i]; } + /** Returns the number of traits. */ public int size() { return traits.length; } http://git-wip-us.apache.org/repos/asf/calcite/blob/de9a7164/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java b/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java index 1f0bdf8..43b44d4 100644 --- a/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java +++ b/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java @@ -19,12 +19,8 @@ package org.apache.calcite.plan; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.convert.ConverterRule; -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; - -import java.util.List; -import javax.annotation.Nonnull; +import com.google.common.collect.Interner; +import com.google.common.collect.Interners; /** * RelTraitDef represents a class of {@link RelTrait}s. Implementations of @@ -56,34 +52,12 @@ import javax.annotation.Nonnull; public abstract class RelTraitDef<T extends RelTrait> { //~ Instance fields -------------------------------------------------------- - private final LoadingCache<T, T> canonicalMap = - CacheBuilder.newBuilder() - .softValues() - .build(CacheLoader.from(key -> key)); - - /** Cache of composite traits. - * - * <p>Uses soft values to allow GC. + /** + * Cache of traits. * - * <p>You can look up using a {@link RelCompositeTrait} whose constituent - * traits are not canonized. + * <p>Uses weak interner to allow GC. */ - private final LoadingCache<Object, RelCompositeTrait> canonicalCompositeMap = - CacheBuilder.newBuilder() - .softValues() - .build( - new CacheLoader<Object, RelCompositeTrait>() { - @Override public RelCompositeTrait load(@Nonnull Object key) { - if (key instanceof RelCompositeTrait) { - return (RelCompositeTrait) key; - } - @SuppressWarnings("unchecked") - final List<RelMultipleTrait> list = - (List<RelMultipleTrait>) key; - final RelTraitDef def = list.get(0).getTraitDef(); - return (RelCompositeTrait) RelCompositeTrait.of(def, list); - } - }); + private final Interner<T> interner = Interners.newWeakInterner(); //~ Constructors ----------------------------------------------------------- @@ -126,20 +100,13 @@ public abstract class RelTraitDef<T extends RelTrait> { * @return a canonical RelTrait. */ public final T canonize(T trait) { - if (trait instanceof RelCompositeTrait) { - RelCompositeTrait relCompositeTrait = (RelCompositeTrait) trait; - return (T) canonizeComposite(relCompositeTrait); + if (!(trait instanceof RelCompositeTrait)) { + assert getTraitClass().isInstance(trait) + : getClass().getName() + + " cannot canonize a " + + trait.getClass().getName(); } - assert getTraitClass().isInstance(trait) - : getClass().getName() - + " cannot canonize a " - + trait.getClass().getName(); - - return canonicalMap.getUnchecked(trait); - } - - final RelCompositeTrait canonizeComposite(RelCompositeTrait compositeTrait) { - return canonicalCompositeMap.getUnchecked(compositeTrait); + return interner.intern(trait); } /**
