From b5210ca95c4303bd91fdb9cb1d063f5680183f0b Mon Sep 17 00:00:00 2001 From: benyu Date: Fri, 10 Jul 2020 15:32:35 -0700 Subject: [PATCH] Speed up Traverser and cut about 30 lines of code. When stacked against the unsubmitted Walker, before the change: breadthFirst was roughly on par; preOrder/postOrder were about 70% slower (1359 vs. 2358); After adopting the Walker impl, the tree traversal preorder/postorder are improved close to Walker impl. There is still about 10% slowness (2405 vs. 2268) remaining, which I suspect is due to Iterator being slower than Spliterator, because with Spliterator, we can tryAdvance() once for each element, while with Iterator, we have to call both hasNext() and next(). The graph traversal adoption is similar, with about 15% remaining slowness compared to Walker (1583 vs. 1338), which is likely result of Spliterator.tryAdvance() vs. Iterator.hasNext() + next(). Did not adopt the Walker's breadth-first impl for the following reasons: 1. Adopting the Walker's breadth-first impl contributed about 10% slowdown compared to the current impl. I think this is likely due to the eager foreach loop of the successors in the current breadth-first iterator. In the full traversal benchmark, it's likely faster than consuming the successor iterator lazily. On the other hand, the breadth-first iterator is inconsistent with the depth-first iterators that consume the successor iterators lazily. For follow-up: It might be better to go complete lazy for breadth-first, even at the cost of 10% slowdown in the full-traversal benchmark. Plus we can reuse code and delete the two existing BreadthFirstIterator classes. I'm going to add the benchmark class in the the labs directory to compare between Iteration and Traverser. RELNOTES=Optimize Traverser ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=320688457 --- .../google/common/graph/TraverserTest.java | 12 +- .../com/google/common/graph/Traverser.java | 207 ++++++++---------- .../google/common/graph/TraverserTest.java | 12 +- .../com/google/common/graph/Traverser.java | 207 ++++++++---------- 4 files changed, 196 insertions(+), 242 deletions(-) diff --git a/android/guava-tests/test/com/google/common/graph/TraverserTest.java b/android/guava-tests/test/com/google/common/graph/TraverserTest.java index f1db94300a9c..497c43bf33b7 100644 --- a/android/guava-tests/test/com/google/common/graph/TraverserTest.java +++ b/android/guava-tests/test/com/google/common/graph/TraverserTest.java @@ -519,11 +519,11 @@ public void forGraph_depthFirstPreOrder_iterableIsLazy() { Iterable result = Traverser.forGraph(graph).depthFirstPreOrder('a'); assertEqualCharNodes(Iterables.limit(result, 2), "ab"); - assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'd'); + assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b'); // Iterate again to see if calculation is done again assertEqualCharNodes(Iterables.limit(result, 2), "ab"); - assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'd', 'd'); + assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b'); } @Test @@ -532,11 +532,11 @@ public void forGraph_depthFirstPreOrderIterable_iterableIsLazy() { Iterable result = Traverser.forGraph(graph).depthFirstPreOrder(charactersOf("ac")); assertEqualCharNodes(Iterables.limit(result, 2), "ab"); - assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'c', 'd'); + assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'c'); // Iterate again to see if calculation is done again assertEqualCharNodes(Iterables.limit(result, 2), "ab"); - assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'c', 'd', 'd'); + assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'c'); } @Test @@ -1022,11 +1022,11 @@ public void forTree_depthFirstPreOrder_iterableIsLazy() { Iterable result = Traverser.forGraph(graph).depthFirstPreOrder('h'); assertEqualCharNodes(Iterables.limit(result, 2), "hd"); - assertThat(graph.requestedNodes).containsExactly('h', 'h', 'd', 'a'); + assertThat(graph.requestedNodes).containsExactly('h', 'h', 'd'); // Iterate again to see if calculation is done again assertEqualCharNodes(Iterables.limit(result, 2), "hd"); - assertThat(graph.requestedNodes).containsExactly('h', 'h', 'h', 'd', 'd', 'a', 'a'); + assertThat(graph.requestedNodes).containsExactly('h', 'h', 'h', 'd', 'd'); } @Test diff --git a/android/guava/src/com/google/common/graph/Traverser.java b/android/guava/src/com/google/common/graph/Traverser.java index 713e54b05b5b..40c3d2a68ccd 100644 --- a/android/guava/src/com/google/common/graph/Traverser.java +++ b/android/guava/src/com/google/common/graph/Traverser.java @@ -363,7 +363,7 @@ public Iterable depthFirstPreOrder(final Iterable startNodes) { return new Iterable() { @Override public Iterator iterator() { - return new DepthFirstIterator(startNodes, Order.PREORDER); + return Walker.inGraph(graph).preOrder(startNodes.iterator()); } }; } @@ -386,7 +386,7 @@ public Iterable depthFirstPostOrder(final Iterable startNodes) { return new Iterable() { @Override public Iterator iterator() { - return new DepthFirstIterator(startNodes, Order.POSTORDER); + return Walker.inGraph(graph).postOrder(startNodes.iterator()); } }; } @@ -427,58 +427,6 @@ public N next() { return current; } } - - private final class DepthFirstIterator extends AbstractIterator { - private final Deque stack = new ArrayDeque<>(); - private final Set visited = new HashSet<>(); - private final Order order; - - DepthFirstIterator(Iterable roots, Order order) { - stack.push(new NodeAndSuccessors(null, roots)); - this.order = order; - } - - @Override - protected N computeNext() { - while (true) { - if (stack.isEmpty()) { - return endOfData(); - } - NodeAndSuccessors nodeAndSuccessors = stack.getFirst(); - boolean firstVisit = visited.add(nodeAndSuccessors.node); - boolean lastVisit = !nodeAndSuccessors.successorIterator.hasNext(); - boolean produceNode = - (firstVisit && order == Order.PREORDER) || (lastVisit && order == Order.POSTORDER); - if (lastVisit) { - stack.pop(); - } else { - // we need to push a neighbor, but only if we haven't already seen it - N successor = nodeAndSuccessors.successorIterator.next(); - if (!visited.contains(successor)) { - stack.push(withSuccessors(successor)); - } - } - if (produceNode && nodeAndSuccessors.node != null) { - return nodeAndSuccessors.node; - } - } - } - - NodeAndSuccessors withSuccessors(N node) { - return new NodeAndSuccessors(node, graph.successors(node)); - } - - /** A simple tuple of a node and a partially iterated {@link Iterator} of its successors. */ - private final class NodeAndSuccessors { - @NullableDecl final N node; - final Iterator successorIterator; - - NodeAndSuccessors(@NullableDecl N node, Iterable successors) { - this.node = node; - this.successorIterator = successors.iterator(); - } - } - } } private static final class TreeTraverser extends Traverser { @@ -529,7 +477,7 @@ public Iterable depthFirstPreOrder(final Iterable startNodes) { return new Iterable() { @Override public Iterator iterator() { - return new DepthFirstPreOrderIterator(startNodes); + return Walker.inTree(tree).preOrder(startNodes.iterator()); } }; } @@ -552,7 +500,7 @@ public Iterable depthFirstPostOrder(final Iterable startNodes) { return new Iterable() { @Override public Iterator iterator() { - return new DepthFirstPostOrderIterator(startNodes); + return Walker.inTree(tree).postOrder(startNodes.iterator()); } }; } @@ -585,77 +533,106 @@ public N next() { return current; } } + } - private final class DepthFirstPreOrderIterator extends UnmodifiableIterator { - private final Deque> stack = new ArrayDeque<>(); - - DepthFirstPreOrderIterator(Iterable roots) { - stack.addLast(roots.iterator()); - } - - @Override - public boolean hasNext() { - return !stack.isEmpty(); - } + /** + * Abstracts away the difference between traversing a graph vs. a tree. For a tree, we just take + * the next element from the next non-empty iterator; for graph, we need to loop through the next + * non-empty iterator to find first unvisited node. + */ + private abstract static class Walker { + final SuccessorsFunction successorFunction; - @Override - public N next() { - Iterator iterator = stack.getLast(); // throws NoSuchElementException if empty - N result = checkNotNull(iterator.next()); - if (!iterator.hasNext()) { - stack.removeLast(); - } - Iterator childIterator = tree.successors(result).iterator(); - if (childIterator.hasNext()) { - stack.addLast(childIterator); - } - return result; - } + Walker(SuccessorsFunction successorFunction) { + this.successorFunction = checkNotNull(successorFunction); } - private final class DepthFirstPostOrderIterator extends AbstractIterator { - private final ArrayDeque stack = new ArrayDeque<>(); - - DepthFirstPostOrderIterator(Iterable roots) { - stack.addLast(new NodeAndChildren(null, roots)); - } - - @Override - protected N computeNext() { - while (!stack.isEmpty()) { - NodeAndChildren top = stack.getLast(); - if (top.childIterator.hasNext()) { - N child = top.childIterator.next(); - stack.addLast(withChildren(child)); - } else { - stack.removeLast(); - if (top.node != null) { - return top.node; + static Walker inGraph(SuccessorsFunction graph) { + final Set visited = new HashSet<>(); + return new Walker(graph) { + @Override + N visitNext(Deque> horizon) { + Iterator top = horizon.getFirst(); + while (top.hasNext()) { + N element = checkNotNull(top.next()); + if (visited.add(element)) { + return element; } } + horizon.removeFirst(); + return null; } - return endOfData(); - } + }; + } - NodeAndChildren withChildren(N node) { - return new NodeAndChildren(node, tree.successors(node)); - } + static Walker inTree(SuccessorsFunction tree) { + return new Walker(tree) { + @Override + N visitNext(Deque> horizon) { + Iterator top = horizon.getFirst(); + if (top.hasNext()) { + return checkNotNull(top.next()); + } + horizon.removeFirst(); + return null; + } + }; + } - /** A simple tuple of a node and a partially iterated {@link Iterator} of its children. */ - private final class NodeAndChildren { - @NullableDecl final N node; - final Iterator childIterator; + final Iterator preOrder(Iterator startNodes) { + final Deque> horizon = new ArrayDeque<>(); + horizon.addFirst(startNodes); + return new AbstractIterator() { + @Override + protected N computeNext() { + do { + N next = visitNext(horizon); + if (next != null) { + Iterator successors = successorFunction.successors(next).iterator(); + if (successors.hasNext()) { + horizon.addFirst(successors); + } + return next; + } + } while (!horizon.isEmpty()); + return endOfData(); + } + }; + } - NodeAndChildren(@NullableDecl N node, Iterable children) { - this.node = node; - this.childIterator = children.iterator(); + final Iterator postOrder(Iterator startNodes) { + final Deque> horizon = new ArrayDeque<>(); + horizon.addFirst(startNodes); + final Deque ancestorStack = new ArrayDeque<>(); + return new AbstractIterator() { + @Override + protected N computeNext() { + for (N next = visitNext(horizon); next != null; next = visitNext(horizon)) { + Iterator successors = successorFunction.successors(next).iterator(); + if (!successors.hasNext()) { + return next; + } + horizon.addFirst(successors); + ancestorStack.push(next); + } + return ancestorStack.isEmpty() ? endOfData() : ancestorStack.pop(); } - } + }; } - } - private enum Order { - PREORDER, - POSTORDER + /** + * Visits the next node from the top iterator of {@code horizon} and returns the visited node. + * Null is returned to indicate reaching the end of the top iterator, which can be used by the + * traversal strategies to decide what to return in such case: in pre-order, continue to poll + * the next top iterator with {@code visitNext()}; in post-order, return the parent node. + * + *

For example, if horizon is {@code [[a, b], [c, d], [e]]}, {@code visitNext()} will return + * {@code [a, b, null, c, d, null, e, null]} sequentially, encoding the topological structure. + * (Note, however, that the callers of {@code visitNext()} often insert additional iterators + * into {@code horizon} between calls to {@code visitNext()}. This causes them to receive + * additional values interleaved with those shown above.) + */ + @NullableDecl + abstract N visitNext(Deque> horizon); } } diff --git a/guava-tests/test/com/google/common/graph/TraverserTest.java b/guava-tests/test/com/google/common/graph/TraverserTest.java index f1db94300a9c..497c43bf33b7 100644 --- a/guava-tests/test/com/google/common/graph/TraverserTest.java +++ b/guava-tests/test/com/google/common/graph/TraverserTest.java @@ -519,11 +519,11 @@ public void forGraph_depthFirstPreOrder_iterableIsLazy() { Iterable result = Traverser.forGraph(graph).depthFirstPreOrder('a'); assertEqualCharNodes(Iterables.limit(result, 2), "ab"); - assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'd'); + assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b'); // Iterate again to see if calculation is done again assertEqualCharNodes(Iterables.limit(result, 2), "ab"); - assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'd', 'd'); + assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b'); } @Test @@ -532,11 +532,11 @@ public void forGraph_depthFirstPreOrderIterable_iterableIsLazy() { Iterable result = Traverser.forGraph(graph).depthFirstPreOrder(charactersOf("ac")); assertEqualCharNodes(Iterables.limit(result, 2), "ab"); - assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'c', 'd'); + assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'c'); // Iterate again to see if calculation is done again assertEqualCharNodes(Iterables.limit(result, 2), "ab"); - assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'c', 'd', 'd'); + assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'c'); } @Test @@ -1022,11 +1022,11 @@ public void forTree_depthFirstPreOrder_iterableIsLazy() { Iterable result = Traverser.forGraph(graph).depthFirstPreOrder('h'); assertEqualCharNodes(Iterables.limit(result, 2), "hd"); - assertThat(graph.requestedNodes).containsExactly('h', 'h', 'd', 'a'); + assertThat(graph.requestedNodes).containsExactly('h', 'h', 'd'); // Iterate again to see if calculation is done again assertEqualCharNodes(Iterables.limit(result, 2), "hd"); - assertThat(graph.requestedNodes).containsExactly('h', 'h', 'h', 'd', 'd', 'a', 'a'); + assertThat(graph.requestedNodes).containsExactly('h', 'h', 'h', 'd', 'd'); } @Test diff --git a/guava/src/com/google/common/graph/Traverser.java b/guava/src/com/google/common/graph/Traverser.java index 1a53dc673dc3..258346020528 100644 --- a/guava/src/com/google/common/graph/Traverser.java +++ b/guava/src/com/google/common/graph/Traverser.java @@ -363,7 +363,7 @@ public Iterable depthFirstPreOrder(final Iterable startNodes) { return new Iterable() { @Override public Iterator iterator() { - return new DepthFirstIterator(startNodes, Order.PREORDER); + return Walker.inGraph(graph).preOrder(startNodes.iterator()); } }; } @@ -386,7 +386,7 @@ public Iterable depthFirstPostOrder(final Iterable startNodes) { return new Iterable() { @Override public Iterator iterator() { - return new DepthFirstIterator(startNodes, Order.POSTORDER); + return Walker.inGraph(graph).postOrder(startNodes.iterator()); } }; } @@ -427,58 +427,6 @@ public N next() { return current; } } - - private final class DepthFirstIterator extends AbstractIterator { - private final Deque stack = new ArrayDeque<>(); - private final Set visited = new HashSet<>(); - private final Order order; - - DepthFirstIterator(Iterable roots, Order order) { - stack.push(new NodeAndSuccessors(null, roots)); - this.order = order; - } - - @Override - protected N computeNext() { - while (true) { - if (stack.isEmpty()) { - return endOfData(); - } - NodeAndSuccessors nodeAndSuccessors = stack.getFirst(); - boolean firstVisit = visited.add(nodeAndSuccessors.node); - boolean lastVisit = !nodeAndSuccessors.successorIterator.hasNext(); - boolean produceNode = - (firstVisit && order == Order.PREORDER) || (lastVisit && order == Order.POSTORDER); - if (lastVisit) { - stack.pop(); - } else { - // we need to push a neighbor, but only if we haven't already seen it - N successor = nodeAndSuccessors.successorIterator.next(); - if (!visited.contains(successor)) { - stack.push(withSuccessors(successor)); - } - } - if (produceNode && nodeAndSuccessors.node != null) { - return nodeAndSuccessors.node; - } - } - } - - NodeAndSuccessors withSuccessors(N node) { - return new NodeAndSuccessors(node, graph.successors(node)); - } - - /** A simple tuple of a node and a partially iterated {@link Iterator} of its successors. */ - private final class NodeAndSuccessors { - final @Nullable N node; - final Iterator successorIterator; - - NodeAndSuccessors(@Nullable N node, Iterable successors) { - this.node = node; - this.successorIterator = successors.iterator(); - } - } - } } private static final class TreeTraverser extends Traverser { @@ -529,7 +477,7 @@ public Iterable depthFirstPreOrder(final Iterable startNodes) { return new Iterable() { @Override public Iterator iterator() { - return new DepthFirstPreOrderIterator(startNodes); + return Walker.inTree(tree).preOrder(startNodes.iterator()); } }; } @@ -552,7 +500,7 @@ public Iterable depthFirstPostOrder(final Iterable startNodes) { return new Iterable() { @Override public Iterator iterator() { - return new DepthFirstPostOrderIterator(startNodes); + return Walker.inTree(tree).postOrder(startNodes.iterator()); } }; } @@ -585,77 +533,106 @@ public N next() { return current; } } + } - private final class DepthFirstPreOrderIterator extends UnmodifiableIterator { - private final Deque> stack = new ArrayDeque<>(); - - DepthFirstPreOrderIterator(Iterable roots) { - stack.addLast(roots.iterator()); - } - - @Override - public boolean hasNext() { - return !stack.isEmpty(); - } + /** + * Abstracts away the difference between traversing a graph vs. a tree. For a tree, we just take + * the next element from the next non-empty iterator; for graph, we need to loop through the next + * non-empty iterator to find first unvisited node. + */ + private abstract static class Walker { + final SuccessorsFunction successorFunction; - @Override - public N next() { - Iterator iterator = stack.getLast(); // throws NoSuchElementException if empty - N result = checkNotNull(iterator.next()); - if (!iterator.hasNext()) { - stack.removeLast(); - } - Iterator childIterator = tree.successors(result).iterator(); - if (childIterator.hasNext()) { - stack.addLast(childIterator); - } - return result; - } + Walker(SuccessorsFunction successorFunction) { + this.successorFunction = checkNotNull(successorFunction); } - private final class DepthFirstPostOrderIterator extends AbstractIterator { - private final ArrayDeque stack = new ArrayDeque<>(); - - DepthFirstPostOrderIterator(Iterable roots) { - stack.addLast(new NodeAndChildren(null, roots)); - } - - @Override - protected N computeNext() { - while (!stack.isEmpty()) { - NodeAndChildren top = stack.getLast(); - if (top.childIterator.hasNext()) { - N child = top.childIterator.next(); - stack.addLast(withChildren(child)); - } else { - stack.removeLast(); - if (top.node != null) { - return top.node; + static Walker inGraph(SuccessorsFunction graph) { + final Set visited = new HashSet<>(); + return new Walker(graph) { + @Override + N visitNext(Deque> horizon) { + Iterator top = horizon.getFirst(); + while (top.hasNext()) { + N element = checkNotNull(top.next()); + if (visited.add(element)) { + return element; } } + horizon.removeFirst(); + return null; } - return endOfData(); - } + }; + } - NodeAndChildren withChildren(N node) { - return new NodeAndChildren(node, tree.successors(node)); - } + static Walker inTree(SuccessorsFunction tree) { + return new Walker(tree) { + @Override + N visitNext(Deque> horizon) { + Iterator top = horizon.getFirst(); + if (top.hasNext()) { + return checkNotNull(top.next()); + } + horizon.removeFirst(); + return null; + } + }; + } - /** A simple tuple of a node and a partially iterated {@link Iterator} of its children. */ - private final class NodeAndChildren { - final @Nullable N node; - final Iterator childIterator; + final Iterator preOrder(Iterator startNodes) { + final Deque> horizon = new ArrayDeque<>(); + horizon.addFirst(startNodes); + return new AbstractIterator() { + @Override + protected N computeNext() { + do { + N next = visitNext(horizon); + if (next != null) { + Iterator successors = successorFunction.successors(next).iterator(); + if (successors.hasNext()) { + horizon.addFirst(successors); + } + return next; + } + } while (!horizon.isEmpty()); + return endOfData(); + } + }; + } - NodeAndChildren(@Nullable N node, Iterable children) { - this.node = node; - this.childIterator = children.iterator(); + final Iterator postOrder(Iterator startNodes) { + final Deque> horizon = new ArrayDeque<>(); + horizon.addFirst(startNodes); + final Deque ancestorStack = new ArrayDeque<>(); + return new AbstractIterator() { + @Override + protected N computeNext() { + for (N next = visitNext(horizon); next != null; next = visitNext(horizon)) { + Iterator successors = successorFunction.successors(next).iterator(); + if (!successors.hasNext()) { + return next; + } + horizon.addFirst(successors); + ancestorStack.push(next); + } + return ancestorStack.isEmpty() ? endOfData() : ancestorStack.pop(); } - } + }; } - } - private enum Order { - PREORDER, - POSTORDER + /** + * Visits the next node from the top iterator of {@code horizon} and returns the visited node. + * Null is returned to indicate reaching the end of the top iterator, which can be used by the + * traversal strategies to decide what to return in such case: in pre-order, continue to poll + * the next top iterator with {@code visitNext()}; in post-order, return the parent node. + * + *

For example, if horizon is {@code [[a, b], [c, d], [e]]}, {@code visitNext()} will return + * {@code [a, b, null, c, d, null, e, null]} sequentially, encoding the topological structure. + * (Note, however, that the callers of {@code visitNext()} often insert additional iterators + * into {@code horizon} between calls to {@code visitNext()}. This causes them to receive + * additional values interleaved with those shown above.) + */ + @Nullable + abstract N visitNext(Deque> horizon); } }