BUG-650: SnapshotBackedWriteTransaction performance

Remove synchronized block from ready() and cleanup exposed interfaces,
adding some task which need to be fixed still.

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

diff --git a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SnapshotBackedReadWriteTransaction.java b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SnapshotBackedReadWriteTransaction.java
index 2ae7425bbb9de443d4cdae61dc90dbc94ba61234..30fa6da58bf99bd47f92758500adb8c317733d57 100644 (file)
--- a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SnapshotBackedReadWriteTransaction.java
+++ b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SnapshotBackedReadWriteTransaction.java
@@ -8,16 +8,13 @@
 package org.opendaylight.controller.md.sal.dom.store.impl;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-
 import com.google.common.base.Optional;
 import com.google.common.util.concurrent.CheckedFuture;
 import com.google.common.util.concurrent.Futures;
-
 import org.opendaylight.controller.md.sal.common.api.data.ReadFailedException;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreReadWriteTransaction;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
-import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeModification;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeSnapshot;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -27,9 +24,7 @@ import org.slf4j.LoggerFactory;
  * and executed according to {@link TransactionReadyPrototype}.
  *
  */
-class SnapshotBackedReadWriteTransaction extends SnapshotBackedWriteTransaction
-                                         implements DOMStoreReadWriteTransaction {
-
+final class SnapshotBackedReadWriteTransaction extends SnapshotBackedWriteTransaction implements DOMStoreReadWriteTransaction {
     private static final Logger LOG = LoggerFactory.getLogger(SnapshotBackedReadWriteTransaction.class);
 
     /**
@@ -49,16 +44,18 @@ class SnapshotBackedReadWriteTransaction extends SnapshotBackedWriteTransaction
         LOG.debug("Tx: {} Read: {}", getIdentifier(), path);
         checkNotNull(path, "Path must not be null.");
 
-        DataTreeModification dataView = getMutatedView();
-        if(dataView == null) {
-            return Futures.immediateFailedCheckedFuture(new ReadFailedException("Transaction is closed"));
-        }
-
+        final Optional<NormalizedNode<?, ?>> result;
         try {
-            return Futures.immediateCheckedFuture(dataView.readNode(path));
+            result = readSnapshotNode(path);
         } catch (Exception e) {
             LOG.error("Tx: {} Failed Read of {}", getIdentifier(), path, e);
-            return Futures.immediateFailedCheckedFuture(new ReadFailedException("Read failed",e));
+            return Futures.immediateFailedCheckedFuture(new ReadFailedException("Read failed", e));
+        }
+
+        if (result == null) {
+            return Futures.immediateFailedCheckedFuture(new ReadFailedException("Transaction is closed"));
+        } else {
+            return Futures.immediateCheckedFuture(result);
         }
     }
 

diff --git a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SnapshotBackedWriteTransaction.java b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SnapshotBackedWriteTransaction.java
index 6129df74787b2fdb8b2da30d9cc2c78f04b288bc..5f102047684f595a81adc12ddd18866f4fbcd9d7 100644 (file)
--- a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SnapshotBackedWriteTransaction.java
+++ b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SnapshotBackedWriteTransaction.java
@@ -8,11 +8,11 @@
 package org.opendaylight.controller.md.sal.dom.store.impl;
 
 import static com.google.common.base.Preconditions.checkState;
-
 import com.google.common.base.Objects.ToStringHelper;
+import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Throwables;
-
+import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreThreePhaseCommitCohort;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreWriteTransaction;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
@@ -29,11 +29,14 @@ import org.slf4j.LoggerFactory;
  *
  */
 class SnapshotBackedWriteTransaction extends AbstractDOMStoreTransaction implements DOMStoreWriteTransaction {
-
     private static final Logger LOG = LoggerFactory.getLogger(SnapshotBackedWriteTransaction.class);
-    private DataTreeModification mutableTree;
-    private boolean ready = false;
-    private TransactionReadyPrototype readyImpl;
+    private static final AtomicReferenceFieldUpdater<SnapshotBackedWriteTransaction, TransactionReadyPrototype> READY_UPDATER =
+            AtomicReferenceFieldUpdater.newUpdater(SnapshotBackedWriteTransaction.class, TransactionReadyPrototype.class, "readyImpl");
+    private static final AtomicReferenceFieldUpdater<SnapshotBackedWriteTransaction, DataTreeModification> TREE_UPDATER =
+            AtomicReferenceFieldUpdater.newUpdater(SnapshotBackedWriteTransaction.class, DataTreeModification.class, "mutableTree");
+
+    private volatile TransactionReadyPrototype readyImpl;        // non-null when not ready
+    private volatile DataTreeModification mutableTree;           // non-null when not committed/closed
 
     /**
      * Creates new write-only transaction.
@@ -48,27 +51,23 @@ class SnapshotBackedWriteTransaction extends AbstractDOMStoreTransaction impleme
     public SnapshotBackedWriteTransaction(final Object identifier, final boolean debug,
             final DataTreeSnapshot snapshot, final TransactionReadyPrototype readyImpl) {
         super(identifier, debug);
-        mutableTree = snapshot.newModification();
         this.readyImpl = Preconditions.checkNotNull(readyImpl, "readyImpl must not be null.");
+        mutableTree = snapshot.newModification();
         LOG.debug("Write Tx: {} allocated with snapshot {}", identifier, snapshot);
     }
 
-    @Override
-    public void close() {
-        LOG.debug("Store transaction: {} : Closed", getIdentifier());
-        this.mutableTree = null;
-        this.readyImpl = null;
-    }
-
     @Override
     public void write(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
         checkNotReady();
+
+        final DataTreeModification tree = mutableTree;
+        LOG.debug("Tx: {} Write: {}:{}", getIdentifier(), path, data);
+
         try {
-            LOG.debug("Tx: {} Write: {}:{}", getIdentifier(), path, data);
-            mutableTree.write(path, data);
+            tree.write(path, data);
             // FIXME: Add checked exception
         } catch (Exception e) {
-            LOG.error("Tx: {}, failed to write {}:{} in {}", getIdentifier(), path, data, mutableTree, e);
+            LOG.error("Tx: {}, failed to write {}:{} in {}", getIdentifier(), path, data, tree, e);
             // Rethrow original ones if they are subclasses of RuntimeException
             // or Error
             Throwables.propagateIfPossible(e);
@@ -80,12 +79,15 @@ class SnapshotBackedWriteTransaction extends AbstractDOMStoreTransaction impleme
     @Override
     public void merge(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
         checkNotReady();
+
+        final DataTreeModification tree = mutableTree;
+        LOG.debug("Tx: {} Merge: {}:{}", getIdentifier(), path, data);
+
         try {
-            LOG.debug("Tx: {} Merge: {}:{}", getIdentifier(), path, data);
-            mutableTree.merge(path, data);
+            tree.merge(path, data);
             // FIXME: Add checked exception
         } catch (Exception e) {
-            LOG.error("Tx: {}, failed to write {}:{} in {}", getIdentifier(), path, data, mutableTree, e);
+            LOG.error("Tx: {}, failed to write {}:{} in {}", getIdentifier(), path, data, tree, e);
             // Rethrow original ones if they are subclasses of RuntimeException
             // or Error
             Throwables.propagateIfPossible(e);
@@ -97,12 +99,15 @@ class SnapshotBackedWriteTransaction extends AbstractDOMStoreTransaction impleme
     @Override
     public void delete(final YangInstanceIdentifier path) {
         checkNotReady();
+
+        final DataTreeModification tree = mutableTree;
+        LOG.debug("Tx: {} Delete: {}", getIdentifier(), path);
+
         try {
-            LOG.debug("Tx: {} Delete: {}", getIdentifier(), path);
-            mutableTree.delete(path);
+            tree.delete(path);
             // FIXME: Add checked exception
         } catch (Exception e) {
-            LOG.error("Tx: {}, failed to delete {} in {}", getIdentifier(), path, mutableTree, e);
+            LOG.error("Tx: {}, failed to delete {} in {}", getIdentifier(), path, tree, e);
             // Rethrow original ones if they are subclasses of RuntimeException
             // or Error
             Throwables.propagateIfPossible(e);
@@ -111,30 +116,54 @@ class SnapshotBackedWriteTransaction extends AbstractDOMStoreTransaction impleme
         }
     }
 
-    protected final boolean isReady() {
-        return ready;
+    /**
+     * Exposed for {@link SnapshotBackedReadWriteTransaction}'s sake only. The contract does
+     * not allow data access after the transaction has been closed or readied.
+     *
+     * @param path Path to read
+     * @return null if the the transaction has been closed;
+     */
+    protected final Optional<NormalizedNode<?, ?>> readSnapshotNode(final YangInstanceIdentifier path) {
+        return readyImpl == null ? null : mutableTree.readNode(path);
     }
 
-    protected final void checkNotReady() {
-        checkState(!ready, "Transaction %s is ready. No further modifications allowed.", getIdentifier());
+    private final void checkNotReady() {
+        checkState(readyImpl != null, "Transaction %s is no longer open. No further modifications allowed.", getIdentifier());
     }
 
     @Override
-    public synchronized DOMStoreThreePhaseCommitCohort ready() {
-        checkState(!ready, "Transaction %s is already ready.", getIdentifier());
-        ready = true;
+    public DOMStoreThreePhaseCommitCohort ready() {
+        final TransactionReadyPrototype wasReady = READY_UPDATER.getAndSet(this, null);
+        checkState(wasReady != null, "Transaction %s is no longer open", getIdentifier());
+
         LOG.debug("Store transaction: {} : Ready", getIdentifier());
         mutableTree.ready();
-        return readyImpl.ready(this);
+        return wasReady.ready(this);
     }
 
-    protected DataTreeModification getMutatedView() {
-        return mutableTree;
+    @Override
+    public void close() {
+        final TransactionReadyPrototype wasReady = READY_UPDATER.getAndSet(this, null);
+        if (wasReady != null) {
+            LOG.debug("Store transaction: {} : Closed", getIdentifier());
+            TREE_UPDATER.lazySet(this, null);
+        } else {
+            LOG.debug("Store transaction: {} : Closed after submit", getIdentifier());
+        }
     }
 
     @Override
     protected ToStringHelper addToStringAttributes(final ToStringHelper toStringHelper) {
-        return toStringHelper.add("ready", isReady());
+        return toStringHelper.add("ready", readyImpl == null);
+    }
+
+    // FIXME: used by chaining on, which really wants an mutated view with a precondition
+    final boolean isReady() {
+        return readyImpl == null;
+    }
+
+    protected DataTreeModification getMutatedView() {
+        return mutableTree;
     }
 
     /**
@@ -146,8 +175,8 @@ class SnapshotBackedWriteTransaction extends AbstractDOMStoreTransaction impleme
      * providing underlying logic for applying implementation.
      *
      */
+    // FIXME: needs access to local stuff, so make it an abstract class
     public static interface TransactionReadyPrototype {
-
         /**
          * Returns a commit coordinator associated with supplied transactions.
          *