Relax DataContainerCodecPrototype.instance locking

All the objects we are creating in createInstance() do not have
unexpected side-effects nor should they require happens-before
semantics beyond what they can guarantee themselves.

Base on that we can make loadInstance() lockless and concurrent,
and reconcile purely via getAcquire()/compareAndExchangeRelease().

JIRA: MDSAL-579
Change-Id: I292024afafa41aecb65e547acc5c888ba7890ee1
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataContainerCodecPrototype.java b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataContainerCodecPrototype.java
index 7cc9f60495205c709c36186e46ce8b2f1e5c33dc..c613aa1cb64d12d51ecf97388e99f584210f42d7 100644 (file)
--- a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataContainerCodecPrototype.java
+++ b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataContainerCodecPrototype.java
@@ -10,7 +10,6 @@ package org.opendaylight.mdsal.binding.dom.codec.impl;
 import com.google.common.collect.Iterables;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.VarHandle;
-import org.checkerframework.checker.lock.qual.Holding;
 import org.eclipse.jdt.annotation.NonNull;
 import org.opendaylight.mdsal.binding.dom.codec.api.BindingDataObjectCodecTreeNode.ChildAddressabilitySummary;
 import org.opendaylight.mdsal.binding.dom.codec.impl.NodeCodecContext.CodecContextFactory;
@@ -59,18 +58,7 @@ final class DataContainerCodecPrototype<T extends WithStatus> implements NodeCon
     private final PathArgument yangArg;
     private final ChildAddressabilitySummary childAddressabilitySummary;
 
-    /*
-     * This field is now utterly in the hot path of CodecDataObject's instantiation of data. It is a volatile support
-     * double-checked locking, as detailed in https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
-     *
-     * We are opting for the document Java 9 equivalent, forsaking our usual CAS-based approach, where we'd re-assert
-     * concurrent loads. This improves safety a bit (by forcing a synchronized() block), as we expect prototypes to be
-     * long-lived.
-     *
-     * FIXME: MDSAL-579: all we need is safe publish semantics here, we can most probably tolerate concurrent value
-     *                   loads -- and everything seems to be ready. This means loadInstance() should not be
-     *                   synchronized, createInstance() should not have @Holding.
-     */
+    // Accessed via INSTANCE
     @SuppressWarnings("unused")
     private volatile DataContainerCodecContext<?, T> instance;
 
@@ -230,27 +218,18 @@ final class DataContainerCodecPrototype<T extends WithStatus> implements NodeCon
 
     @Override
     public DataContainerCodecContext<?, T> get() {
-        final DataContainerCodecContext<?, T> existing = getInstance();
+        final DataContainerCodecContext<?, T> existing = (DataContainerCodecContext<?, T>) INSTANCE.getAcquire(this);
         return existing != null ? existing : loadInstance();
     }
 
-    private synchronized @NonNull DataContainerCodecContext<?, T> loadInstance() {
-        // re-acquire under lock
-        DataContainerCodecContext<?, T> tmp = getInstance();
-        if (tmp == null) {
-            tmp = createInstance();
-            INSTANCE.setRelease(this, tmp);
-        }
-        return tmp;
-    }
-
-    // Helper for getAcquire access to instance
-    private DataContainerCodecContext<?, T> getInstance() {
-        return (DataContainerCodecContext<?, T>) INSTANCE.getAcquire(this);
+    private @NonNull DataContainerCodecContext<?, T> loadInstance() {
+        final DataContainerCodecContext<?, T> tmp = createInstance();
+        final Object witness = INSTANCE.compareAndExchangeRelease(this, null, tmp);
+        return witness == null ? tmp : (DataContainerCodecContext<?, T>) witness;
     }
 
-    @Holding("this")
     @SuppressWarnings({ "rawtypes", "unchecked" })
+    // This method is required to allow concurrent invocation.
     private @NonNull DataContainerCodecContext<?, T> createInstance() {
         // FIXME: make protected abstract
         if (schema instanceof ContainerSchemaNode) {