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 5840c9dbd14d1aa3b3fe050d079e426d8ec21f20..85cdf0335fecc793fc29e0c357f41cf4ee41c1e6 100644 (file)
--- 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;
 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 {
 
 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;
     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 {
         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.
      */
     }
 
     /**
      * Called when schema context changes, invalidates cache.
      */

-    public synchronized void refresh() {
-        cache.clear();
+    public void refresh() {
+        ref.set(new SoftReference<YangStoreSnapshotImpl>(null));

     }
 }
     }
 }