Skip to content

Commit

Permalink
Speed up Traverser and cut about 30 lines of code.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@fluentfuture @netdpb
fluentfuture authored and netdpb committed Jul 13, 2020
1 parent a47ab01 commit b5210ca
Show file tree
Hide file tree
Showing 4 changed files with 196 additions and 242 deletions.
12 changes: 6 additions & 6 deletions android/guava-tests/test/com/google/common/graph/TraverserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,11 +519,11 @@ public void forGraph_depthFirstPreOrder_iterableIsLazy() {
Iterable<Character> 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
Expand All @@ -532,11 +532,11 @@ public void forGraph_depthFirstPreOrderIterable_iterableIsLazy() {
Iterable<Character> 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
Expand Down Expand Up @@ -1022,11 +1022,11 @@ public void forTree_depthFirstPreOrder_iterableIsLazy() {
Iterable<Character> 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
Expand Down
207 changes: 92 additions & 115 deletions android/guava/src/com/google/common/graph/Traverser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ public Iterable<N> depthFirstPreOrder(final Iterable<? extends N> startNodes) {
return new Iterable<N>() {
@Override
public Iterator<N> iterator() {
return new DepthFirstIterator(startNodes, Order.PREORDER);
return Walker.inGraph(graph).preOrder(startNodes.iterator());
}
};
}
Expand All @@ -386,7 +386,7 @@ public Iterable<N> depthFirstPostOrder(final Iterable<? extends N> startNodes) {
return new Iterable<N>() {
@Override
public Iterator<N> iterator() {
return new DepthFirstIterator(startNodes, Order.POSTORDER);
return Walker.inGraph(graph).postOrder(startNodes.iterator());
}
};
}
Expand Down Expand Up @@ -427,58 +427,6 @@ public N next() {
return current;
}
}

private final class DepthFirstIterator extends AbstractIterator<N> {
private final Deque<NodeAndSuccessors> stack = new ArrayDeque<>();
private final Set<N> visited = new HashSet<>();
private final Order order;

DepthFirstIterator(Iterable<? extends N> 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<? extends N> successorIterator;

NodeAndSuccessors(@NullableDecl N node, Iterable<? extends N> successors) {
this.node = node;
this.successorIterator = successors.iterator();
}
}
}
}

private static final class TreeTraverser<N> extends Traverser<N> {
Expand Down Expand Up @@ -529,7 +477,7 @@ public Iterable<N> depthFirstPreOrder(final Iterable<? extends N> startNodes) {
return new Iterable<N>() {
@Override
public Iterator<N> iterator() {
return new DepthFirstPreOrderIterator(startNodes);
return Walker.inTree(tree).preOrder(startNodes.iterator());
}
};
}
Expand All @@ -552,7 +500,7 @@ public Iterable<N> depthFirstPostOrder(final Iterable<? extends N> startNodes) {
return new Iterable<N>() {
@Override
public Iterator<N> iterator() {
return new DepthFirstPostOrderIterator(startNodes);
return Walker.inTree(tree).postOrder(startNodes.iterator());
}
};
}
Expand Down Expand Up @@ -585,77 +533,106 @@ public N next() {
return current;
}
}
}

private final class DepthFirstPreOrderIterator extends UnmodifiableIterator<N> {
private final Deque<Iterator<? extends N>> stack = new ArrayDeque<>();

DepthFirstPreOrderIterator(Iterable<? extends N> 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<N> {
final SuccessorsFunction<N> successorFunction;

@Override
public N next() {
Iterator<? extends N> iterator = stack.getLast(); // throws NoSuchElementException if empty
N result = checkNotNull(iterator.next());
if (!iterator.hasNext()) {
stack.removeLast();
}
Iterator<? extends N> childIterator = tree.successors(result).iterator();
if (childIterator.hasNext()) {
stack.addLast(childIterator);
}
return result;
}
Walker(SuccessorsFunction<N> successorFunction) {
this.successorFunction = checkNotNull(successorFunction);
}

private final class DepthFirstPostOrderIterator extends AbstractIterator<N> {
private final ArrayDeque<NodeAndChildren> stack = new ArrayDeque<>();

DepthFirstPostOrderIterator(Iterable<? extends N> 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 <N> Walker<N> inGraph(SuccessorsFunction<N> graph) {
final Set<N> visited = new HashSet<>();
return new Walker<N>(graph) {
@Override
N visitNext(Deque<Iterator<? extends N>> horizon) {
Iterator<? extends N> 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 <N> Walker<N> inTree(SuccessorsFunction<N> tree) {
return new Walker<N>(tree) {
@Override
N visitNext(Deque<Iterator<? extends N>> horizon) {
Iterator<? extends N> 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<? extends N> childIterator;
final Iterator<N> preOrder(Iterator<? extends N> startNodes) {
final Deque<Iterator<? extends N>> horizon = new ArrayDeque<>();
horizon.addFirst(startNodes);
return new AbstractIterator<N>() {
@Override
protected N computeNext() {
do {
N next = visitNext(horizon);
if (next != null) {
Iterator<? extends N> successors = successorFunction.successors(next).iterator();
if (successors.hasNext()) {
horizon.addFirst(successors);
}
return next;
}
} while (!horizon.isEmpty());
return endOfData();
}
};
}

NodeAndChildren(@NullableDecl N node, Iterable<? extends N> children) {
this.node = node;
this.childIterator = children.iterator();
final Iterator<N> postOrder(Iterator<? extends N> startNodes) {
final Deque<Iterator<? extends N>> horizon = new ArrayDeque<>();
horizon.addFirst(startNodes);
final Deque<N> ancestorStack = new ArrayDeque<>();
return new AbstractIterator<N>() {
@Override
protected N computeNext() {
for (N next = visitNext(horizon); next != null; next = visitNext(horizon)) {
Iterator<? extends N> 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.
*
* <p>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<Iterator<? extends N>> horizon);
}
}
12 changes: 6 additions & 6 deletions guava-tests/test/com/google/common/graph/TraverserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,11 +519,11 @@ public void forGraph_depthFirstPreOrder_iterableIsLazy() {
Iterable<Character> 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
Expand All @@ -532,11 +532,11 @@ public void forGraph_depthFirstPreOrderIterable_iterableIsLazy() {
Iterable<Character> 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
Expand Down Expand Up @@ -1022,11 +1022,11 @@ public void forTree_depthFirstPreOrder_iterableIsLazy() {
Iterable<Character> 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
Expand Down
Loading

1 comment on commit b5210ca

@jbduncan
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Please sign in to comment.