From 23a3dea5804c8198e61f550736911b5c992d6b6d Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 12 Sep 2014 12:05:14 +0200 Subject: [PATCH] BUG-650: optimize DOMForwardedWriteTransaction First make sure we get visiblity into cohorts as a collections. This allows us to optimize allocation of the list where we hold the futures. We also make it non-immutable -- it is handed off, so there is no reason to pay the price of an additional allocation. Second we optimize the transaction state checking by eliminating as many volatile read/writes as possible. The bottom line is the following: 1) the put/merge/delete paths see only a single volatile read instead of two 2) the submit path performs only one volatile get+set and an ordered write -- instead of two reads and two writes 3) the cancel path performs a volatile get+set and either an ordered write (fast path) or a spin on volatile read (slow path) -- instead of a synchronized block, four volatile reads and two volatile writes Change-Id: I5ec875e65acdee62a4f0daf233617e6af024637f Signed-off-by: Robert Varga --- ...tractDOMForwardedCompositeTransaction.java | 10 +- .../impl/DOMForwardedWriteTransaction.java | 137 ++++++++---------- 2 files changed, 70 insertions(+), 77 deletions(-) diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/AbstractDOMForwardedCompositeTransaction.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/AbstractDOMForwardedCompositeTransaction.java index d3791a0878..c20c3650d2 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/AbstractDOMForwardedCompositeTransaction.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/AbstractDOMForwardedCompositeTransaction.java @@ -6,14 +6,16 @@ */ package org.opendaylight.controller.md.sal.dom.broker.impl; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; + +import java.util.Collection; + import org.opendaylight.controller.md.sal.common.api.data.AsyncTransaction; import org.opendaylight.controller.sal.core.spi.data.DOMStoreTransaction; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; -import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableMap; - /** * Composite DOM Transaction backed by {@link DOMStoreTransaction}. * @@ -66,7 +68,7 @@ abstract class AbstractDOMForwardedCompositeTransaction getSubtransactions() { + protected Collection getSubtransactions() { return backingTxs.values(); } diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransaction.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransaction.java index 5d4ad4d803..8c57cfb18d 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransaction.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransaction.java @@ -7,10 +7,15 @@ */ package org.opendaylight.controller.md.sal.dom.broker.impl; -import static com.google.common.base.Preconditions.checkState; - -import javax.annotation.concurrent.GuardedBy; - +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import com.google.common.util.concurrent.CheckedFuture; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import java.util.ArrayList; +import java.util.Collection; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import org.opendaylight.controller.md.sal.common.api.TransactionStatus; import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType; import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException; @@ -21,18 +26,12 @@ import org.opendaylight.controller.sal.core.spi.data.DOMStoreWriteTransaction; import org.opendaylight.yangtools.yang.common.RpcResult; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; - -import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.util.concurrent.CheckedFuture; -import com.google.common.util.concurrent.ListenableFuture; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** - * - * * Read-Write Transaction, which is composed of several - * {@link DOMStoreWriteTransaction} transactions. Subtransaction is selected by + * {@link DOMStoreWriteTransaction} transactions. A sub-transaction is selected by * {@link LogicalDatastoreType} type parameter in: * *
    @@ -46,38 +45,38 @@ import com.google.common.util.concurrent.ListenableFuture; * invocation with all {@link org.opendaylight.controller.sal.core.spi.data.DOMStoreThreePhaseCommitCohort} for underlying * transactions. * - * @param - * Subtype of {@link DOMStoreWriteTransaction} which is used as + * @param Subtype of {@link DOMStoreWriteTransaction} which is used as * subtransaction. */ class DOMForwardedWriteTransaction extends AbstractDOMForwardedCompositeTransaction implements DOMDataWriteTransaction { + @SuppressWarnings("rawtypes") + private static final AtomicReferenceFieldUpdater IMPL_UPDATER = + AtomicReferenceFieldUpdater.newUpdater(DOMForwardedWriteTransaction.class, DOMDataCommitImplementation.class, "commitImpl"); + @SuppressWarnings("rawtypes") + private static final AtomicReferenceFieldUpdater FUTURE_UPDATER = + AtomicReferenceFieldUpdater.newUpdater(DOMForwardedWriteTransaction.class, Future.class, "commitFuture"); + private static final Logger LOG = LoggerFactory.getLogger(DOMForwardedWriteTransaction.class); + private static final Future CANCELLED_FUTURE = Futures.immediateCancelledFuture(); /** - * Implementation of real commit. - * - * Transaction can not be commited if commitImpl is null, - * so this seting this property to null is also used to - * prevent write to - * already commited / canceled transaction {@link #checkNotCanceled() - * - * + * Implementation of real commit. It also acts as an indication that + * the transaction is running -- which we flip atomically using + * {@link #IMPL_UPDATER}. */ - @GuardedBy("this") private volatile DOMDataCommitImplementation commitImpl; /** + * Future task of transaction commit. It starts off as null, but is + * set appropriately on {@link #submit()} and {@link #cancel()} via + * {@link AtomicReferenceFieldUpdater#lazySet(Object, Object)}. * - * Future task of transaction commit. - * - * This value is initially null, and is once updated if transaction - * is commited {@link #commit()}. - * If this future exists, transaction MUST not be commited again - * and all modifications should fail. See {@link #checkNotCommited()}. - * + * Lazy set is safe for use because it is only referenced to in the + * {@link #cancel()} slow path, where we will busy-wait for it. The + * fast path gets the benefit of a store-store barrier instead of the + * usual store-load barrier. */ - @GuardedBy("this") - private volatile CheckedFuture commitFuture; + private volatile Future commitFuture; protected DOMForwardedWriteTransaction(final Object identifier, final ImmutableMap backingTxs, final DOMDataCommitImplementation commitImpl) { @@ -87,73 +86,65 @@ class DOMForwardedWriteTransaction extends @Override public void put(final LogicalDatastoreType store, final YangInstanceIdentifier path, final NormalizedNode data) { - checkNotReady(); + checkRunning(commitImpl); getSubtransaction(store).write(path, data); } @Override public void delete(final LogicalDatastoreType store, final YangInstanceIdentifier path) { - checkNotReady(); + checkRunning(commitImpl); getSubtransaction(store).delete(path); } @Override public void merge(final LogicalDatastoreType store, final YangInstanceIdentifier path, final NormalizedNode data) { - checkNotReady(); + checkRunning(commitImpl); getSubtransaction(store).merge(path, data); } @Override - public synchronized boolean cancel() { - // Transaction is already canceled, we are safe to return true - final boolean cancelationResult; - if (commitImpl == null && commitFuture != null) { - // Transaction is submitted, we try to cancel future. - cancelationResult = commitFuture.cancel(false); - } else if(commitImpl == null) { + public boolean cancel() { + final DOMDataCommitImplementation impl = IMPL_UPDATER.getAndSet(this, null); + if (impl != null) { + LOG.trace("Transaction {} cancelled before submit", getIdentifier()); + FUTURE_UPDATER.lazySet(this, CANCELLED_FUTURE); return true; - } else { - cancelationResult = true; - commitImpl = null; } - return cancelationResult; + // The transaction is in process of being submitted or cancelled. Busy-wait + // for the corresponding future. + Future future; + do { + future = commitFuture; + } while (future == null); + + return future.cancel(false); } @Override - public synchronized ListenableFuture> commit() { + public ListenableFuture> commit() { return AbstractDataTransaction.convertToLegacyCommitFuture(submit()); } @Override - public CheckedFuture submit() { - checkNotReady(); + public CheckedFuture submit() { + final DOMDataCommitImplementation impl = IMPL_UPDATER.getAndSet(this, null); + checkRunning(impl); - ImmutableList.Builder cohortsBuilder = ImmutableList.builder(); - for (DOMStoreWriteTransaction subTx : getSubtransactions()) { - cohortsBuilder.add(subTx.ready()); - } - ImmutableList cohorts = cohortsBuilder.build(); - commitFuture = commitImpl.submit(this, cohorts); - - /* - *We remove reference to Commit Implementation in order - *to prevent memory leak - */ - commitImpl = null; - return commitFuture; - } + final Collection txns = getSubtransactions(); + final Collection cohorts = new ArrayList<>(txns.size()); - private void checkNotReady() { - checkNotCommited(); - checkNotCanceled(); - } + // FIXME: deal with errors thrown by backed (ready and submit can fail in theory) + for (DOMStoreWriteTransaction txn : txns) { + cohorts.add(txn.ready()); + } - private void checkNotCanceled() { - Preconditions.checkState(commitImpl != null, "Transaction was canceled."); + final CheckedFuture ret = impl.submit(this, cohorts); + FUTURE_UPDATER.lazySet(this, ret); + return ret; } - private void checkNotCommited() { - checkState(commitFuture == null, "Transaction was already submited."); + private void checkRunning(final DOMDataCommitImplementation impl) { + Preconditions.checkState(impl != null, "Transaction %s is no longer running", getIdentifier()); } -} \ No newline at end of file +} -- 2.36.6