Improve ShardedDOMDataTreeProducer locking

The path from backend was taking coarse lock even when not needed.
Explicitly annotate submitTransaction() as needing the lock being
held and push down its acquisition so we take it only after we
are certain we actually need it.

Change-Id: Icd1226796568829ea3735e6eec42677a79b9b3b5
Signed-off-by: Robert Varga <rovarga@cisco.com>
(cherry picked from commit 2ce13c8680d68ba1e50eab9609069b6aecfa62a5)

diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeProducer.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeProducer.java
index b19d956192f23abd9f992b075a98fd272e1c0678..773bd27b87170652a385cd4f1dac7ef3c9f8c5e9 100644 (file)
--- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeProducer.java
+++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeProducer.java
@@ -47,7 +47,6 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer {
     private static final AtomicReferenceFieldUpdater<ShardedDOMDataTreeProducer, ShardedDOMDataTreeWriteTransaction>
         CURRENT_UPDATER = AtomicReferenceFieldUpdater.newUpdater(ShardedDOMDataTreeProducer.class,
             ShardedDOMDataTreeWriteTransaction.class, "currentTx");
-    @SuppressWarnings("unused")
     private volatile ShardedDOMDataTreeWriteTransaction currentTx;
 
     private static final AtomicReferenceFieldUpdater<ShardedDOMDataTreeProducer, ShardedDOMDataTreeWriteTransaction>
@@ -155,6 +154,7 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer {
         return ret;
     }
 
+    @GuardedBy("this")
     private void submitTransaction(final ShardedDOMDataTreeWriteTransaction current) {
         lastTx = current;
         current.doSubmit(this::transactionSuccessful, this::transactionFailed);
@@ -251,47 +251,61 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer {
         }
     }
 
-    void processTransaction(final ShardedDOMDataTreeWriteTransaction transaction) {
+    // Called when the user submits a transaction
+    void transactionSubmitted(final ShardedDOMDataTreeWriteTransaction transaction) {
         final boolean wasOpen = OPEN_UPDATER.compareAndSet(this, transaction, null);
-        Verify.verify(wasOpen);
-
-        if (lastTx != null) {
-            final boolean success = CURRENT_UPDATER.compareAndSet(this, null, transaction);
-            Verify.verify(success);
-            if (lastTx == null) {
-                // Dispatch after requeue
-                processCurrentTransaction();
+        Preconditions.checkState(wasOpen, "Attempted to submit non-open transaction %s", transaction);
+
+        if (lastTx == null) {
+            // No transaction outstanding, we need to submit it now
+            synchronized (this) {
+                submitTransaction(transaction);
             }
-        } else {
-            submitTransaction(transaction);
+
+            return;
+        }
+
+        // There is a potentially-racing submitted transaction. Publish the new one, which may end up being
+        // picked up by processNextTransaction.
+        final boolean success = CURRENT_UPDATER.compareAndSet(this, null, transaction);
+        Verify.verify(success);
+
+        // Now a quick check: if the racing transaction completed in between, it may have missed the current
+        // transaction, hence we need to re-check
+        if (lastTx == null) {
+            submitCurrentTransaction();
         }
     }
 
-    void transactionSuccessful(final ShardedDOMDataTreeWriteTransaction tx) {
+    private void submitCurrentTransaction() {
+        final ShardedDOMDataTreeWriteTransaction current = currentTx;
+        if (current != null) {
+            synchronized (this) {
+                if (CURRENT_UPDATER.compareAndSet(this, current, null)) {
+                    submitTransaction(current);
+                }
+            }
+        }
+    }
+
+    private void transactionSuccessful(final ShardedDOMDataTreeWriteTransaction tx) {
         LOG.debug("Transaction {} completed successfully", tx.getIdentifier());
 
         tx.onTransactionSuccess(null);
-        processNextTransaction(tx);
+        transactionCompleted(tx);
     }
 
-    void transactionFailed(final ShardedDOMDataTreeWriteTransaction tx, final Throwable throwable) {
+    private void transactionFailed(final ShardedDOMDataTreeWriteTransaction tx, final Throwable throwable) {
         LOG.debug("Transaction {} failed", tx.getIdentifier(), throwable);
 
         tx.onTransactionFailure(throwable);
-        processNextTransaction(tx);
-    }
-
-    private void processCurrentTransaction() {
-        final ShardedDOMDataTreeWriteTransaction current = CURRENT_UPDATER.getAndSet(this, null);
-        if (current != null) {
-            submitTransaction(current);
-        }
+        transactionCompleted(tx);
     }
 
-    private synchronized void processNextTransaction(final ShardedDOMDataTreeWriteTransaction tx) {
+    private void transactionCompleted(final ShardedDOMDataTreeWriteTransaction tx) {
         final boolean wasLast = LAST_UPDATER.compareAndSet(this, tx, null);
         if (wasLast) {
-            processCurrentTransaction();
+            submitCurrentTransaction();
         }
     }
 

diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeWriteTransaction.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeWriteTransaction.java
index 83080f7be141b5129b84a32bb0cc62bd595e034b..fcbc9169fca9759f7ba8f5c910f2525a850ec047 100644 (file)
--- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeWriteTransaction.java
+++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeWriteTransaction.java
@@ -125,7 +125,7 @@ final class ShardedDOMDataTreeWriteTransaction implements DOMDataTreeCursorAware
         Preconditions.checkState(!closed, "Transaction %s is already closed", identifier);
         Preconditions.checkState(openCursor == null, "Cannot submit transaction while there is a cursor open");
 
-        producer.processTransaction(this);
+        producer.transactionSubmitted(this);
         return submitFuture;
     }