From 5f693add15c8702d72e0018ef2d30af076a5e537 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sat, 17 Sep 2016 00:33:35 +0200 Subject: [PATCH] Improve ShardedDOMDataTreeProducer locking Making 'closed' a CAS-capable field allows us to check state and transition to closed state without holding the object's lock. This allows associated critical sections to be reduced to the extent as to make the reuse fast-path completely lock-free. Change-Id: I29bacebf5d37a38ea6b4cc641f43dc369cd9edcf Signed-off-by: Robert Varga (cherry picked from commit 6f0f4aeb4562db98368051b3ab7382ea6af8baf8) --- .../broker/ShardedDOMDataTreeProducer.java | 114 ++++++++++-------- 1 file changed, 66 insertions(+), 48 deletions(-) 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 773bd27b87..022215a6b2 100644 --- 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 @@ -21,6 +21,7 @@ import java.util.Collection; import java.util.Map; import java.util.Map.Entry; import java.util.Set; +import java.util.concurrent.atomic.AtomicIntegerFieldUpdater; import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import javax.annotation.concurrent.GuardedBy; import org.opendaylight.mdsal.dom.api.DOMDataTreeCursorAwareTransaction; @@ -59,12 +60,14 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer { ShardedDOMDataTreeWriteTransaction.class, "lastTx"); private volatile ShardedDOMDataTreeWriteTransaction lastTx; + private static final AtomicIntegerFieldUpdater CLOSED_UPDATER = + AtomicIntegerFieldUpdater.newUpdater(ShardedDOMDataTreeProducer.class, "closed"); + private volatile int closed; + @GuardedBy("this") private Map children = ImmutableMap.of(); @GuardedBy("this") private Set childRoots = ImmutableSet.of(); - @GuardedBy("this") - private boolean closed; @GuardedBy("this") private ShardedDOMDataTreeListenerContext attachedListener; @@ -93,17 +96,6 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer { return new ShardedDOMDataTreeProducer(dataTree, subtrees, shardMap, shardToIdentifiers); } - void subshardAdded(final Map shardMap) { - Preconditions.checkState(openTx == null, "Transaction %s is still open", openTx); - final Multimap shardToIdentifiers = ArrayListMultimap.create(); - // map which identifier belongs to which shard - for (final Entry entry : shardMap.entrySet()) { - shardToIdentifiers.put(entry.getValue(), entry.getKey()); - } - this.idToProducer = mapIdsToProducer(shardToIdentifiers); - idToShard = ImmutableMap.copyOf(shardMap); - } - private static BiMap mapIdsToProducer( final Multimap shardToId) { final Builder idToProducerBuilder = ImmutableBiMap.builder(); @@ -124,33 +116,58 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer { return idToProducerBuilder.build(); } - @Override - public synchronized DOMDataTreeCursorAwareTransaction createTransaction(final boolean isolated) { - Preconditions.checkState(!closed, "Producer is already closed"); + private void checkNotClosed() { + Preconditions.checkState(closed == 0, "Producer is already closed"); + } + + private void checkIdle() { Preconditions.checkState(openTx == null, "Transaction %s is still open", openTx); + } + + void subshardAdded(final Map shardMap) { + checkIdle(); + + final Multimap shardToIdentifiers = ArrayListMultimap.create(); + // map which identifier belongs to which shard + for (final Entry entry : shardMap.entrySet()) { + shardToIdentifiers.put(entry.getValue(), entry.getKey()); + } + this.idToProducer = mapIdsToProducer(shardToIdentifiers); + idToShard = ImmutableMap.copyOf(shardMap); + } + + @Override + public DOMDataTreeCursorAwareTransaction createTransaction(final boolean isolated) { + checkNotClosed(); + checkIdle(); - LOG.debug("Creating transaction from producer"); - final ShardedDOMDataTreeWriteTransaction current = CURRENT_UPDATER.getAndSet(this, null); final ShardedDOMDataTreeWriteTransaction ret; + LOG.debug("Creating transaction from producer {}", this); + + final ShardedDOMDataTreeWriteTransaction current = CURRENT_UPDATER.getAndSet(this, null); if (isolated) { // Isolated case. If we have a previous transaction, submit it before returning this one. - if (current != null) { - submitTransaction(current); + synchronized (this) { + if (current != null) { + submitTransaction(current); + } + ret = new ShardedDOMDataTreeWriteTransaction(this, idToProducer, childRoots); } - ret = new ShardedDOMDataTreeWriteTransaction(this, idToProducer, childRoots); } else { // Non-isolated case, see if we can reuse the transaction if (current != null) { LOG.debug("Reusing previous transaction {} since there is still a transaction inflight", - current.getIdentifier()); + current.getIdentifier()); ret = current; } else { - ret = new ShardedDOMDataTreeWriteTransaction(this, idToProducer, childRoots); + synchronized (this) { + ret = new ShardedDOMDataTreeWriteTransaction(this, idToProducer, childRoots); + } } } final boolean success = OPEN_UPDATER.compareAndSet(this, null, ret); - Verify.verify(success); + Preconditions.checkState(success, "Illegal concurrent access to producer %s detected", this); return ret; } @@ -183,9 +200,9 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer { } @Override - public synchronized DOMDataTreeProducer createProducer(final Collection subtrees) { - Preconditions.checkState(!closed, "Producer is already closed"); - Preconditions.checkState(openTx == null, "Transaction %s is still open", openTx); + public DOMDataTreeProducer createProducer(final Collection subtrees) { + checkNotClosed(); + checkIdle(); for (final DOMDataTreeIdentifier s : subtrees) { // Check if the subtree was visible at any time @@ -196,24 +213,24 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer { // Check if part of the requested subtree is not delegated to a child. for (final DOMDataTreeIdentifier c : children.keySet()) { - if (s.contains(c)) { - throw new IllegalArgumentException(String.format("Subtree %s cannot be delegated as it is" - + " superset of already-delegated %s", s, c)); - } + Preconditions.checkArgument(!s.contains(c), + "Subtree %s cannot be delegated as it is a superset of already-delegated %s", s, c); } } - final DOMDataTreeProducer ret = dataTree.createProducer(this, subtrees); - final ImmutableMap.Builder cb = ImmutableMap.builder(); - cb.putAll(children); - for (final DOMDataTreeIdentifier s : subtrees) { - cb.put(s, ret); - } + synchronized (this) { + final DOMDataTreeProducer ret = dataTree.createProducer(this, subtrees); + final ImmutableMap.Builder cb = ImmutableMap.builder(); + cb.putAll(children); + for (final DOMDataTreeIdentifier s : subtrees) { + cb.put(s, ret); + } - children = cb.build(); - childRoots = ImmutableSet.copyOf(Collections2.transform(children.keySet(), - DOMDataTreeIdentifier::getRootIdentifier)); - return ret; + children = cb.build(); + childRoots = ImmutableSet.copyOf(Collections2.transform(children.keySet(), + DOMDataTreeIdentifier::getRootIdentifier)); + return ret; + } } boolean isDelegatedToChild(final DOMDataTreeIdentifier path) { @@ -227,14 +244,15 @@ class ShardedDOMDataTreeProducer implements DOMDataTreeProducer { @Override - public synchronized void close() throws DOMDataTreeProducerException { - if (!closed) { - if (openTx != null) { - throw new DOMDataTreeProducerBusyException(String.format("Transaction %s is still open", openTx)); - } + public void close() throws DOMDataTreeProducerException { + if (openTx != null) { + throw new DOMDataTreeProducerBusyException(String.format("Transaction %s is still open", openTx)); + } - closed = true; - dataTree.destroyProducer(this); + if (CLOSED_UPDATER.compareAndSet(this, 0, 1)) { + synchronized (this) { + dataTree.destroyProducer(this); + } } } -- 2.36.6