From: Robert Varga <rovarga@cisco.com>
Date: Thu, 17 Jul 2014 14:19:32 +0000 (+0200)
Subject: BUG-1384: YangStoreServiceImpl.refresh() should never block
X-Git-Tag: release/helium~470^2
X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=bca9e7b16cd96a988be8b9feeb1a738d12f9ca25

BUG-1384: YangStoreServiceImpl.refresh() should never block

As descirbed in the bug, we are seeing large amounts of contention on
the monitor when calling refresh(). Rework the safety such that the
refresh path does not block and perform checking for refresh in the
get() path, which is forced to retry should a refresh occur.

With this patch reported contention disappears completely and startup
time is improved by about 8 seconds on stock SP edition.

Change-Id: I4c6109d77324d3d1b700041b54a95dc346e3d372
Signed-off-by: Robert Varga <rovarga@cisco.com>
---

diff --git a/opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/osgi/YangStoreServiceImpl.java b/opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/osgi/YangStoreServiceImpl.java
index 5840c9dbd1..85cdf0335f 100644
--- a/opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/osgi/YangStoreServiceImpl.java
+++ b/opendaylight/netconf/config-netconf-connector/src/main/java/org/opendaylight/controller/netconf/confignetconfconnector/osgi/YangStoreServiceImpl.java
@@ -9,33 +9,65 @@
 package org.opendaylight.controller.netconf.confignetconfconnector.osgi;
 
 import java.lang.ref.SoftReference;
-import org.opendaylight.yangtools.yang.model.api.SchemaContextProvider;
+import java.util.concurrent.atomic.AtomicReference;
 
-import javax.annotation.concurrent.GuardedBy;
+import org.opendaylight.yangtools.yang.model.api.SchemaContextProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class YangStoreServiceImpl implements YangStoreService {
+    private static final Logger LOG = LoggerFactory.getLogger(YangStoreServiceImpl.class);
+
+    /**
+     * This is a rather interesting locking model. We need to guard against both the
+     * cache expiring from GC and being invalidated by schema context change. The
+     * context can change while we are doing processing, so we do not want to block
+     * it, so no synchronization can happen on the methods.
+     *
+     * So what we are doing is the following:
+     *
+     * We synchronize with GC as usual, using a SoftReference.
+     *
+     * The atomic reference is used to synchronize with {@link #refresh()}, e.g. when
+     * refresh happens, it will push a SoftReference(null), e.g. simulate the GC. Now
+     * that may happen while the getter is already busy acting on the old schema context,
+     * so it needs to understand that a refresh has happened and retry. To do that, it
+     * attempts a CAS operation -- if it fails, in knows that the SoftReference has
+     * been replaced and thus it needs to retry.
+     *
+     * Note that {@link #getYangStoreSnapshot()} will still use synchronize() internally
+     * to stop multiple threads doing the same work.
+     */
+    private final AtomicReference<SoftReference<YangStoreSnapshotImpl>> ref = new AtomicReference<>(new SoftReference<YangStoreSnapshotImpl>(null));
     private final SchemaContextProvider service;
-    @GuardedBy("this")
-    private SoftReference<YangStoreSnapshotImpl> cache = new SoftReference<>(null);
 
-    public YangStoreServiceImpl(SchemaContextProvider service) {
+    public YangStoreServiceImpl(final SchemaContextProvider service) {
         this.service = service;
     }
 
     @Override
     public synchronized YangStoreSnapshotImpl getYangStoreSnapshot() throws YangStoreException {
-        YangStoreSnapshotImpl yangStoreSnapshot = cache.get();
-        if (yangStoreSnapshot == null) {
-            yangStoreSnapshot = new YangStoreSnapshotImpl(service.getSchemaContext());
-            cache = new SoftReference<>(yangStoreSnapshot);
+        SoftReference<YangStoreSnapshotImpl> r = ref.get();
+        YangStoreSnapshotImpl ret = r.get();
+
+        while (ret == null) {
+            // We need to be compute a new value
+            ret = new YangStoreSnapshotImpl(service.getSchemaContext());
+
+            if (!ref.compareAndSet(r, new SoftReference<>(ret))) {
+                LOG.debug("Concurrent refresh detected, recomputing snapshot");
+                r = ref.get();
+                ret = null;
+            }
         }
-        return yangStoreSnapshot;
+
+        return ret;
     }
 
     /**
      * Called when schema context changes, invalidates cache.
      */
-    public synchronized void refresh() {
-        cache.clear();
+    public void refresh() {
+        ref.set(new SoftReference<YangStoreSnapshotImpl>(null));
     }
 }