BUG-4808: Improve caching of effective attributes

Instead of reusing effective attributes for a particular update, maintain a cache for the duration of peer's role.
This way attributes are cached across multiple messages, maintaining memory efficiency in face of fragmented updates.
The cache maintains weak references only, hence does not prevent garbage collection of stale entries.

Change-Id: I625d2b129814772030ff478b4ec99b71e9b98d37
Signed-off-by: Om Prakash <op317q@att.com>
(cherry picked from commit 5d0d40aec50b865604bd419bbeb0930808207765)

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/CachingImportPolicy.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/CachingImportPolicy.java
index 2a73f1cd2c2cce819fc607f60755dfd01c151d9d..3cf20b24aa6b0bac7b2e90ccedb9c46487ea9dd4 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/CachingImportPolicy.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/CachingImportPolicy.java
@@ -8,11 +8,21 @@
 package org.opendaylight.protocol.bgp.rib.impl;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.Interner;
+import com.google.common.collect.Interners;
+import com.google.common.collect.MapMaker;
+
 import java.util.IdentityHashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentMap;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import javax.annotation.concurrent.NotThreadSafe;
 import org.opendaylight.protocol.bgp.rib.impl.spi.AbstractImportPolicy;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.rib.tables.Attributes;
 import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode;
+import org.opendaylight.yangtools.yang.data.impl.schema.ImmutableNodes;
 
 /**
  * A caching decorator for {@link AbstractImportPolicy}. Performs caching of effective
@@ -21,23 +31,75 @@ import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode;
  */
 @NotThreadSafe
 final class CachingImportPolicy extends AbstractImportPolicy {
-    private final Map<ContainerNode, ContainerNode> cache = new IdentityHashMap<>();
+
+    // A dummy ContainerNode, stored in the cache to indicate null effective attributes
+    private static final ContainerNode MASKED_NULL = ImmutableNodes.containerNode(Attributes.QNAME);
+
+    // We maintain a weak cache of returned effective attributes, so we end up reusing
+    // the same instance when asked. We set concurrency level to 1, as we do not expect
+    // the cache to be accessed from multiple threads. That may need to be changed
+    // if we end up sharing the cache across peers.
+    private final ConcurrentMap<ContainerNode, ContainerNode> cache =
+            new MapMaker().concurrencyLevel(1).weakKeys().weakValues().makeMap();
+
+    /*
+     * The cache itself is weak, which means we end up with identity hash/comparisons.
+     * That is good, but we want the cache to be effective even when equivalent attributes
+     * are presented. For that purpose we maintain a weak interner, which will allow us
+     * to map attributes to a canonical object without preventing garbage collection.
+     */
+    private final Interner<ContainerNode> interner = Interners.newWeakInterner();
+
     private final AbstractImportPolicy delegate;
 
     CachingImportPolicy(final AbstractImportPolicy delegate) {
         this.delegate = Preconditions.checkNotNull(delegate);
     }
 
+    @Nonnull private static ContainerNode maskNull(@Nullable final ContainerNode unmasked) {
+        return unmasked == null ? MASKED_NULL : unmasked;
+    }
+
+    @Nullable private static ContainerNode unmaskNull(@Nonnull final ContainerNode masked) {
+        return MASKED_NULL.equals(masked) ? null : masked;
+    }
+
     @Override
     public ContainerNode effectiveAttributes(final ContainerNode attributes) {
         ContainerNode ret = this.cache.get(attributes);
-        if (ret == null) {
-            ret = this.delegate.effectiveAttributes(attributes);
-            if (ret != null) {
-                this.cache.put(attributes, ret);
+        if (ret != null) {
+            return unmaskNull(ret);
+        }
+
+        /*
+         * The cache returned empty. The reason for that may be that the attributes
+         * passed in are not identical to the ones forming the cache's key. Intern
+         * the passed attributes, which will result in a canonical reference.
+         *
+         * If the returned reference is different, attempt to look up in the cache
+         * again. If the reference is the same, we have just populated the interner
+         * and thus are on the path to create a new cache entry.
+         */
+        final ContainerNode interned = interner.intern(attributes);
+        if (interned != attributes) {
+            final ContainerNode retry = cache.get(interned);
+            if (retry != null) {
+                return unmaskNull(retry);
             }
         }
 
-        return ret;
+        final ContainerNode effective = delegate.effectiveAttributes(interned);
+
+        /*
+         * Populate the cache. Note that this may have raced with another thread,
+         * in which case we want to reuse the previous entry without replacing it.
+         * Check the result of conditional put and return it unmasked if it happens
+         * to be non-null. That will throw away the attributes we just created,
+         * but that's fine, as they have not leaked to heap yet and will be GC'd
+         * quickly.
+         */
+        final ContainerNode existing = cache.putIfAbsent(interned, maskNull(effective));
+        return existing != null ? unmaskNull(existing) : effective;
+
     }
 }

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ImportPolicyPeerTrackerImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ImportPolicyPeerTrackerImpl.java
index ac8c3b8eab7d6349bb6d6c9ffe6f1599803810f7..d1cf0cb2f6672e78af3d17067247e7e8f3c97f75 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ImportPolicyPeerTrackerImpl.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ImportPolicyPeerTrackerImpl.java
@@ -50,6 +50,6 @@ final class ImportPolicyPeerTrackerImpl implements ImportPolicyPeerTracker {
     @Override
     public AbstractImportPolicy policyFor(final PeerId peerId) {
         LOG.trace("Peer ID : {}", peerId);
-        return new CachingImportPolicy(this.policies.get(peerId));
+        return this.policies.get(peerId);
     }
 }
\ No newline at end of file

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/PolicyDatabase.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/PolicyDatabase.java
index f93cfabe694a3e8bf7020c3cde1d001da4e90b1d..a376928fc2a0d7a1dc38e55d9921130ebed943c2 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/PolicyDatabase.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/PolicyDatabase.java
@@ -39,6 +39,14 @@ final class PolicyDatabase {
     }
 
     AbstractImportPolicy importPolicyForRole(final PeerRole peerRole) {
-        return this.importPolicies.get(peerRole);
+        /*
+         * TODO: this solution does not share equivalent attributes across
+         *       multiple peers. If we need to do that, consider carefully:
+         *       - whether the interner should be shared with RIBSupportContextImpl.writeRoutes()
+         *       - lookup/update contention of both the cache and the interner
+         *       - ability to share resulting attributes across import policies
+         *       - ability to share resulting attributes with export policies
+         */
+        return new CachingImportPolicy(importPolicies.get(peerRole));
     }
 }