From d042a4bad5c1cd4a8907529677425573039fc04f Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Thu, 21 Aug 2014 14:54:37 +0200 Subject: [PATCH] BUG-1511 - Datastore: cleanup ListenerTreeAPI - split ListenerTree two static inner classes to ListenerWalker and ListenerNode - fix refences [ResolvedDataChangeState] - improve performance of ListenerWalker.close() Change-Id: Ide82bc6caeaca1aa5c9080f134d570cf7a1571d6 Signed-off-by: Vaclav Demcak Signed-off-by: Robert Varga --- .../impl/ResolveDataChangeEventsTask.java | 4 +- .../store/impl/ResolveDataChangeState.java | 30 ++-- .../sal/dom/store/impl/tree/ListenerNode.java | 106 +++++++++++++ .../sal/dom/store/impl/tree/ListenerTree.java | 143 +----------------- .../dom/store/impl/tree/ListenerWalker.java | 44 ++++++ 5 files changed, 175 insertions(+), 152 deletions(-) create mode 100644 opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerNode.java create mode 100644 opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerWalker.java diff --git a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeEventsTask.java b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeEventsTask.java index 25ddbf5df2..14d565c1d0 100644 --- a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeEventsTask.java +++ b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeEventsTask.java @@ -17,7 +17,7 @@ import org.opendaylight.controller.md.sal.common.api.data.AsyncDataBroker.DataCh import org.opendaylight.controller.md.sal.dom.store.impl.DOMImmutableDataChangeEvent.Builder; import org.opendaylight.controller.md.sal.dom.store.impl.DOMImmutableDataChangeEvent.SimpleEventFactory; import org.opendaylight.controller.md.sal.dom.store.impl.tree.ListenerTree; -import org.opendaylight.controller.md.sal.dom.store.impl.tree.ListenerTree.Walker; +import org.opendaylight.controller.md.sal.dom.store.impl.tree.ListenerWalker; import org.opendaylight.yangtools.util.concurrent.NotificationManager; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; @@ -51,7 +51,7 @@ final class ResolveDataChangeEventsTask { * Resolves and submits notification tasks to the specified manager. */ public synchronized void resolve(final NotificationManager, DOMImmutableDataChangeEvent> manager) { - try (final Walker w = listenerRoot.getWalker()) { + try (final ListenerWalker w = listenerRoot.getWalker()) { // Defensive: reset internal state collectedEvents = ArrayListMultimap.create(); diff --git a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeState.java b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeState.java index 3db4115af6..6bbed57f39 100644 --- a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeState.java +++ b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeState.java @@ -7,11 +7,6 @@ */ package org.opendaylight.controller.md.sal.dom.store.impl; -import com.google.common.base.Optional; -import com.google.common.base.Preconditions; -import com.google.common.collect.Iterables; -import com.google.common.collect.Multimap; - import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -22,7 +17,7 @@ import java.util.Map.Entry; import org.opendaylight.controller.md.sal.common.api.data.AsyncDataBroker.DataChangeScope; import org.opendaylight.controller.md.sal.dom.store.impl.DOMImmutableDataChangeEvent.Builder; -import org.opendaylight.controller.md.sal.dom.store.impl.tree.ListenerTree.Node; +import org.opendaylight.controller.md.sal.dom.store.impl.tree.ListenerNode; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifier; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifierWithPredicates; @@ -32,6 +27,11 @@ import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.base.Optional; +import com.google.common.base.Preconditions; +import com.google.common.collect.Iterables; +import com.google.common.collect.Multimap; + /** * Recursion state used in {@link ResolveDataChangeEventsTask}. Instances of this * method track which listeners are affected by a particular change node. It takes @@ -49,7 +49,7 @@ final class ResolveDataChangeState { */ private final Collection inheritedOne; private final YangInstanceIdentifier nodeId; - private final Collection nodes; + private final Collection nodes; private final Map, Builder> subBuilders; private final Map, Builder> oneBuilders; @@ -57,7 +57,7 @@ final class ResolveDataChangeState { private ResolveDataChangeState(final YangInstanceIdentifier nodeId, final Iterable inheritedSub, final Collection inheritedOne, - final Collection nodes) { + final Collection nodes) { this.nodeId = Preconditions.checkNotNull(nodeId); this.nodes = Preconditions.checkNotNull(nodes); this.inheritedSub = Preconditions.checkNotNull(inheritedSub); @@ -69,7 +69,7 @@ final class ResolveDataChangeState { final Map, Builder> sub = new HashMap<>(); final Map, Builder> one = new HashMap<>(); final Map, Builder> base = new HashMap<>(); - for (Node n : nodes) { + for (ListenerNode n : nodes) { for (DataChangeListenerRegistration l : n.getListeners()) { final Builder b = DOMImmutableDataChangeEvent.builder(DataChangeScope.BASE); switch (l.getScope()) { @@ -105,7 +105,7 @@ final class ResolveDataChangeState { * @param root root node * @return */ - public static ResolveDataChangeState initial(final YangInstanceIdentifier rootId, final Node root) { + public static ResolveDataChangeState initial(final YangInstanceIdentifier rootId, final ListenerNode root) { return new ResolveDataChangeState(rootId, Collections.emptyList(), Collections.emptyList(), Collections.singletonList(root)); } @@ -257,13 +257,13 @@ final class ResolveDataChangeState { LOG.trace("Collected events {}", map); } - private static Collection getListenerChildrenWildcarded(final Collection parentNodes, + private static Collection getListenerChildrenWildcarded(final Collection parentNodes, final PathArgument child) { if (parentNodes.isEmpty()) { return Collections.emptyList(); } - final List result = new ArrayList<>(); + final List result = new ArrayList<>(); if (child instanceof NodeWithValue || child instanceof NodeIdentifierWithPredicates) { NodeIdentifier wildcardedIdentifier = new NodeIdentifier(child.getNodeType()); addChildNodes(result, parentNodes, wildcardedIdentifier); @@ -272,9 +272,9 @@ final class ResolveDataChangeState { return result; } - private static void addChildNodes(final List result, final Collection parentNodes, final PathArgument childIdentifier) { - for (Node node : parentNodes) { - Optional child = node.getChild(childIdentifier); + private static void addChildNodes(final List result, final Collection parentNodes, final PathArgument childIdentifier) { + for (ListenerNode node : parentNodes) { + Optional child = node.getChild(childIdentifier); if (child.isPresent()) { result.add(child.get()); } diff --git a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerNode.java b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerNode.java new file mode 100644 index 0000000000..0aef1429c4 --- /dev/null +++ b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerNode.java @@ -0,0 +1,106 @@ +/** + * Copyright (c) 2014 Cisco Systems, Inc. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ +package org.opendaylight.controller.md.sal.dom.store.impl.tree; + +import com.google.common.base.Optional; +import java.lang.ref.Reference; +import java.lang.ref.WeakReference; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; +import org.opendaylight.controller.md.sal.dom.store.impl.DataChangeListenerRegistration; +import org.opendaylight.controller.md.sal.dom.store.impl.tree.ListenerTree.DataChangeListenerRegistrationImpl; +import org.opendaylight.yangtools.concepts.Identifiable; +import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument; +import org.opendaylight.yangtools.yang.data.api.schema.tree.StoreTreeNode; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This is a single node within the listener tree. Note that the data returned from + * and instance of this class is guaranteed to have any relevance or consistency + * only as long as the {@link ListenerWalker} instance through which it is reached remains + * unclosed. + * + * @author Robert Varga + */ +public class ListenerNode implements StoreTreeNode, Identifiable { + + private static final Logger LOG = LoggerFactory.getLogger(ListenerNode.class); + + private final Collection> listeners = new ArrayList<>(); + private final Map children = new HashMap<>(); + private final PathArgument identifier; + private final Reference parent; + + ListenerNode(final ListenerNode parent, final PathArgument identifier) { + this.parent = new WeakReference<>(parent); + this.identifier = identifier; + } + + @Override + public PathArgument getIdentifier() { + return identifier; + } + + @Override + public Optional getChild(final PathArgument child) { + return Optional.fromNullable(children.get(child)); + } + + /** + * Return the list of current listeners. This collection is guaranteed + * to be immutable only while the walker, through which this node is + * reachable remains unclosed. + * + * @return the list of current listeners + */ + public Collection> getListeners() { + return listeners; + } + + ListenerNode ensureChild(final PathArgument child) { + ListenerNode potential = children.get(child); + if (potential == null) { + potential = new ListenerNode(this, child); + children.put(child, potential); + } + return potential; + } + + void addListener(final DataChangeListenerRegistration listener) { + listeners.add(listener); + LOG.debug("Listener {} registered", listener); + } + + void removeListener(final DataChangeListenerRegistrationImpl listener) { + listeners.remove(listener); + LOG.debug("Listener {} unregistered", listener); + + // We have been called with the write-lock held, so we can perform some cleanup. + removeThisIfUnused(); + } + + private void removeThisIfUnused() { + final ListenerNode p = parent.get(); + if (p != null && listeners.isEmpty() && children.isEmpty()) { + p.removeChild(identifier); + } + } + + private void removeChild(final PathArgument arg) { + children.remove(arg); + removeThisIfUnused(); + } + + @Override + public String toString() { + return "Node [identifier=" + identifier + ", listeners=" + listeners.size() + ", children=" + children.size() + "]"; + } +} diff --git a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerTree.java b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerTree.java index ac7a318187..dcff6439d6 100644 --- a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerTree.java +++ b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerTree.java @@ -7,41 +7,29 @@ */ package org.opendaylight.controller.md.sal.dom.store.impl.tree; -import com.google.common.base.Optional; -import com.google.common.base.Preconditions; - -import java.lang.ref.Reference; -import java.lang.ref.WeakReference; -import java.util.ArrayList; -import java.util.Collection; -import java.util.HashMap; -import java.util.Map; -import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; -import javax.annotation.concurrent.GuardedBy; - import org.opendaylight.controller.md.sal.common.api.data.AsyncDataBroker.DataChangeScope; import org.opendaylight.controller.md.sal.common.api.data.AsyncDataChangeListener; import org.opendaylight.controller.md.sal.dom.store.impl.DataChangeListenerRegistration; import org.opendaylight.yangtools.concepts.AbstractListenerRegistration; -import org.opendaylight.yangtools.concepts.Identifiable; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; -import org.opendaylight.yangtools.yang.data.api.schema.tree.StoreTreeNode; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** * A set of listeners organized as a tree by node to which they listen. This class * allows for efficient lookup of listeners when we walk the DataTreeCandidate. + * + * @author Robert Varga */ public final class ListenerTree { private static final Logger LOG = LoggerFactory.getLogger(ListenerTree.class); private final ReadWriteLock rwLock = new ReentrantReadWriteLock(true); - private final Node rootNode = new Node(null, null); + private final ListenerNode rootNode = new ListenerNode(null, null); private ListenerTree() { // Private to disallow direct instantiation @@ -71,12 +59,12 @@ public final class ListenerTree { rwLock.writeLock().lock(); try { - Node walkNode = rootNode; + ListenerNode walkNode = rootNode; for (final PathArgument arg : path.getPathArguments()) { walkNode = walkNode.ensureChild(arg); } - final Node node = walkNode; + final ListenerNode node = walkNode; DataChangeListenerRegistration reg = new DataChangeListenerRegistrationImpl(listener) { @Override public DataChangeScope getScope() { @@ -128,7 +116,7 @@ public final class ListenerTree { * * @return A walker instance. */ - public Walker getWalker() { + public ListenerWalker getWalker() { /* * TODO: The only current user of this method is local to the datastore. * Since this class represents a read-lock, losing a reference to @@ -137,127 +125,12 @@ public final class ListenerTree { * external user exist, make the Walker a phantom reference, which * will cleanup the lock if not told to do so. */ - final Walker ret = new Walker(rwLock.readLock(), rootNode); + final ListenerWalker ret = new ListenerWalker(rwLock.readLock(), rootNode); rwLock.readLock().lock(); return ret; } - /** - * A walking context, pretty much equivalent to an iterator, but it - * exposes the underlying tree structure. - */ - /* - * FIXME: BUG-1511: split this class out as ListenerWalker. - */ - public static final class Walker implements AutoCloseable { - private final Lock lock; - private final Node node; - - @GuardedBy("this") - private boolean valid = true; - - private Walker(final Lock lock, final Node node) { - this.lock = Preconditions.checkNotNull(lock); - this.node = Preconditions.checkNotNull(node); - } - - public Node getRootNode() { - return node; - } - - @Override - public synchronized void close() { - if (valid) { - lock.unlock(); - valid = false; - } - } - } - - /** - * This is a single node within the listener tree. Note that the data returned from - * and instance of this class is guaranteed to have any relevance or consistency - * only as long as the {@link org.opendaylight.controller.md.sal.dom.store.impl.tree.ListenerTree.Walker} instance through which it is reached remains - * unclosed. - */ - /* - * FIXME: BUG-1511: split this class out as ListenerNode. - */ - public static final class Node implements StoreTreeNode, Identifiable { - private final Collection> listeners = new ArrayList<>(); - private final Map children = new HashMap<>(); - private final PathArgument identifier; - private final Reference parent; - - private Node(final Node parent, final PathArgument identifier) { - this.parent = new WeakReference<>(parent); - this.identifier = identifier; - } - - @Override - public PathArgument getIdentifier() { - return identifier; - } - - @Override - public Optional getChild(final PathArgument child) { - return Optional.fromNullable(children.get(child)); - } - - /** - * Return the list of current listeners. This collection is guaranteed - * to be immutable only while the walker, through which this node is - * reachable remains unclosed. - * - * @return the list of current listeners - */ - public Collection> getListeners() { - return listeners; - } - - private Node ensureChild(final PathArgument child) { - Node potential = children.get(child); - if (potential == null) { - potential = new Node(this, child); - children.put(child, potential); - } - return potential; - } - - private void addListener(final DataChangeListenerRegistration listener) { - listeners.add(listener); - LOG.debug("Listener {} registered", listener); - } - - private void removeListener(final DataChangeListenerRegistrationImpl listener) { - listeners.remove(listener); - LOG.debug("Listener {} unregistered", listener); - - // We have been called with the write-lock held, so we can perform some cleanup. - removeThisIfUnused(); - } - - private void removeThisIfUnused() { - final Node p = parent.get(); - if (p != null && listeners.isEmpty() && children.isEmpty()) { - p.removeChild(identifier); - } - } - - private void removeChild(final PathArgument arg) { - children.remove(arg); - removeThisIfUnused(); - } - - @Override - public String toString() { - return "Node [identifier=" + identifier + ", listeners=" + listeners.size() + ", children=" + children.size() + "]"; - } - - - } - - private abstract static class DataChangeListenerRegistrationImpl>> extends AbstractListenerRegistration // + abstract static class DataChangeListenerRegistrationImpl>> extends AbstractListenerRegistration // implements DataChangeListenerRegistration { public DataChangeListenerRegistrationImpl(final T listener) { super(listener); diff --git a/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerWalker.java b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerWalker.java new file mode 100644 index 0000000000..0c297a2e2b --- /dev/null +++ b/opendaylight/md-sal/sal-inmemory-datastore/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerWalker.java @@ -0,0 +1,44 @@ +/** + * Copyright (c) 2014 Cisco Systems, Inc. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ +package org.opendaylight.controller.md.sal.dom.store.impl.tree; + +import com.google.common.base.Preconditions; +import java.util.concurrent.atomic.AtomicIntegerFieldUpdater; +import java.util.concurrent.locks.Lock; + +/** + * A walking context, pretty much equivalent to an iterator, but it + * exposes the underlying tree structure. + * + * @author Robert Varga + */ +public class ListenerWalker implements AutoCloseable { + private static final AtomicIntegerFieldUpdater CLOSED_UPDATER = AtomicIntegerFieldUpdater.newUpdater(ListenerWalker.class, "closed"); + private final Lock lock; + private final ListenerNode node; + + // Used via CLOSED_UPDATER + @SuppressWarnings("unused") + private volatile int closed = 0; + + ListenerWalker(final Lock lock, final ListenerNode node) { + this.lock = Preconditions.checkNotNull(lock); + this.node = Preconditions.checkNotNull(node); + } + + public ListenerNode getRootNode() { + return node; + } + + @Override + public void close() { + if (CLOSED_UPDATER.compareAndSet(this, 0, 1)) { + lock.unlock(); + } + } +} \ No newline at end of file -- 2.36.6