Skip to content

Commit b5210ca

Browse files
fluentfuturenetdpb
fluentfuture
authored and
netdpb
committed
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
1 parent a47ab01 commit b5210ca

File tree

4 files changed

+196
-242
lines changed

4 files changed

+196
-242
lines changed

‎android/guava-tests/test/com/google/common/graph/TraverserTest.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -519,11 +519,11 @@ public void forGraph_depthFirstPreOrder_iterableIsLazy() {
519519
Iterable<Character> result = Traverser.forGraph(graph).depthFirstPreOrder('a');
520520

521521
assertEqualCharNodes(Iterables.limit(result, 2), "ab");
522-
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'd');
522+
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b');
523523

524524
// Iterate again to see if calculation is done again
525525
assertEqualCharNodes(Iterables.limit(result, 2), "ab");
526-
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'd', 'd');
526+
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b');
527527
}
528528

529529
@Test
@@ -532,11 +532,11 @@ public void forGraph_depthFirstPreOrderIterable_iterableIsLazy() {
532532
Iterable<Character> result = Traverser.forGraph(graph).depthFirstPreOrder(charactersOf("ac"));
533533

534534
assertEqualCharNodes(Iterables.limit(result, 2), "ab");
535-
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'c', 'd');
535+
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'c');
536536

537537
// Iterate again to see if calculation is done again
538538
assertEqualCharNodes(Iterables.limit(result, 2), "ab");
539-
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'c', 'd', 'd');
539+
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'c');
540540
}
541541

542542
@Test
@@ -1022,11 +1022,11 @@ public void forTree_depthFirstPreOrder_iterableIsLazy() {
10221022
Iterable<Character> result = Traverser.forGraph(graph).depthFirstPreOrder('h');
10231023

10241024
assertEqualCharNodes(Iterables.limit(result, 2), "hd");
1025-
assertThat(graph.requestedNodes).containsExactly('h', 'h', 'd', 'a');
1025+
assertThat(graph.requestedNodes).containsExactly('h', 'h', 'd');
10261026

10271027
// Iterate again to see if calculation is done again
10281028
assertEqualCharNodes(Iterables.limit(result, 2), "hd");
1029-
assertThat(graph.requestedNodes).containsExactly('h', 'h', 'h', 'd', 'd', 'a', 'a');
1029+
assertThat(graph.requestedNodes).containsExactly('h', 'h', 'h', 'd', 'd');
10301030
}
10311031

10321032
@Test

‎android/guava/src/com/google/common/graph/Traverser.java

Lines changed: 92 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ public Iterable<N> depthFirstPreOrder(final Iterable<? extends N> startNodes) {
363363
return new Iterable<N>() {
364364
@Override
365365
public Iterator<N> iterator() {
366-
return new DepthFirstIterator(startNodes, Order.PREORDER);
366+
return Walker.inGraph(graph).preOrder(startNodes.iterator());
367367
}
368368
};
369369
}
@@ -386,7 +386,7 @@ public Iterable<N> depthFirstPostOrder(final Iterable<? extends N> startNodes) {
386386
return new Iterable<N>() {
387387
@Override
388388
public Iterator<N> iterator() {
389-
return new DepthFirstIterator(startNodes, Order.POSTORDER);
389+
return Walker.inGraph(graph).postOrder(startNodes.iterator());
390390
}
391391
};
392392
}
@@ -427,58 +427,6 @@ public N next() {
427427
return current;
428428
}
429429
}
430-
431-
private final class DepthFirstIterator extends AbstractIterator<N> {
432-
private final Deque<NodeAndSuccessors> stack = new ArrayDeque<>();
433-
private final Set<N> visited = new HashSet<>();
434-
private final Order order;
435-
436-
DepthFirstIterator(Iterable<? extends N> roots, Order order) {
437-
stack.push(new NodeAndSuccessors(null, roots));
438-
this.order = order;
439-
}
440-
441-
@Override
442-
protected N computeNext() {
443-
while (true) {
444-
if (stack.isEmpty()) {
445-
return endOfData();
446-
}
447-
NodeAndSuccessors nodeAndSuccessors = stack.getFirst();
448-
boolean firstVisit = visited.add(nodeAndSuccessors.node);
449-
boolean lastVisit = !nodeAndSuccessors.successorIterator.hasNext();
450-
boolean produceNode =
451-
(firstVisit && order == Order.PREORDER) || (lastVisit && order == Order.POSTORDER);
452-
if (lastVisit) {
453-
stack.pop();
454-
} else {
455-
// we need to push a neighbor, but only if we haven't already seen it
456-
N successor = nodeAndSuccessors.successorIterator.next();
457-
if (!visited.contains(successor)) {
458-
stack.push(withSuccessors(successor));
459-
}
460-
}
461-
if (produceNode && nodeAndSuccessors.node != null) {
462-
return nodeAndSuccessors.node;
463-
}
464-
}
465-
}
466-
467-
NodeAndSuccessors withSuccessors(N node) {
468-
return new NodeAndSuccessors(node, graph.successors(node));
469-
}
470-
471-
/** A simple tuple of a node and a partially iterated {@link Iterator} of its successors. */
472-
private final class NodeAndSuccessors {
473-
@NullableDecl final N node;
474-
final Iterator<? extends N> successorIterator;
475-
476-
NodeAndSuccessors(@NullableDecl N node, Iterable<? extends N> successors) {
477-
this.node = node;
478-
this.successorIterator = successors.iterator();
479-
}
480-
}
481-
}
482430
}
483431

484432
private static final class TreeTraverser<N> extends Traverser<N> {
@@ -529,7 +477,7 @@ public Iterable<N> depthFirstPreOrder(final Iterable<? extends N> startNodes) {
529477
return new Iterable<N>() {
530478
@Override
531479
public Iterator<N> iterator() {
532-
return new DepthFirstPreOrderIterator(startNodes);
480+
return Walker.inTree(tree).preOrder(startNodes.iterator());
533481
}
534482
};
535483
}
@@ -552,7 +500,7 @@ public Iterable<N> depthFirstPostOrder(final Iterable<? extends N> startNodes) {
552500
return new Iterable<N>() {
553501
@Override
554502
public Iterator<N> iterator() {
555-
return new DepthFirstPostOrderIterator(startNodes);
503+
return Walker.inTree(tree).postOrder(startNodes.iterator());
556504
}
557505
};
558506
}
@@ -585,77 +533,106 @@ public N next() {
585533
return current;
586534
}
587535
}
536+
}
588537

589-
private final class DepthFirstPreOrderIterator extends UnmodifiableIterator<N> {
590-
private final Deque<Iterator<? extends N>> stack = new ArrayDeque<>();
591-
592-
DepthFirstPreOrderIterator(Iterable<? extends N> roots) {
593-
stack.addLast(roots.iterator());
594-
}
595-
596-
@Override
597-
public boolean hasNext() {
598-
return !stack.isEmpty();
599-
}
538+
/**
539+
* Abstracts away the difference between traversing a graph vs. a tree. For a tree, we just take
540+
* the next element from the next non-empty iterator; for graph, we need to loop through the next
541+
* non-empty iterator to find first unvisited node.
542+
*/
543+
private abstract static class Walker<N> {
544+
final SuccessorsFunction<N> successorFunction;
600545

601-
@Override
602-
public N next() {
603-
Iterator<? extends N> iterator = stack.getLast(); // throws NoSuchElementException if empty
604-
N result = checkNotNull(iterator.next());
605-
if (!iterator.hasNext()) {
606-
stack.removeLast();
607-
}
608-
Iterator<? extends N> childIterator = tree.successors(result).iterator();
609-
if (childIterator.hasNext()) {
610-
stack.addLast(childIterator);
611-
}
612-
return result;
613-
}
546+
Walker(SuccessorsFunction<N> successorFunction) {
547+
this.successorFunction = checkNotNull(successorFunction);
614548
}
615549

616-
private final class DepthFirstPostOrderIterator extends AbstractIterator<N> {
617-
private final ArrayDeque<NodeAndChildren> stack = new ArrayDeque<>();
618-
619-
DepthFirstPostOrderIterator(Iterable<? extends N> roots) {
620-
stack.addLast(new NodeAndChildren(null, roots));
621-
}
622-
623-
@Override
624-
protected N computeNext() {
625-
while (!stack.isEmpty()) {
626-
NodeAndChildren top = stack.getLast();
627-
if (top.childIterator.hasNext()) {
628-
N child = top.childIterator.next();
629-
stack.addLast(withChildren(child));
630-
} else {
631-
stack.removeLast();
632-
if (top.node != null) {
633-
return top.node;
550+
static <N> Walker<N> inGraph(SuccessorsFunction<N> graph) {
551+
final Set<N> visited = new HashSet<>();
552+
return new Walker<N>(graph) {
553+
@Override
554+
N visitNext(Deque<Iterator<? extends N>> horizon) {
555+
Iterator<? extends N> top = horizon.getFirst();
556+
while (top.hasNext()) {
557+
N element = checkNotNull(top.next());
558+
if (visited.add(element)) {
559+
return element;
634560
}
635561
}
562+
horizon.removeFirst();
563+
return null;
636564
}
637-
return endOfData();
638-
}
565+
};
566+
}
639567

640-
NodeAndChildren withChildren(N node) {
641-
return new NodeAndChildren(node, tree.successors(node));
642-
}
568+
static <N> Walker<N> inTree(SuccessorsFunction<N> tree) {
569+
return new Walker<N>(tree) {
570+
@Override
571+
N visitNext(Deque<Iterator<? extends N>> horizon) {
572+
Iterator<? extends N> top = horizon.getFirst();
573+
if (top.hasNext()) {
574+
return checkNotNull(top.next());
575+
}
576+
horizon.removeFirst();
577+
return null;
578+
}
579+
};
580+
}
643581

644-
/** A simple tuple of a node and a partially iterated {@link Iterator} of its children. */
645-
private final class NodeAndChildren {
646-
@NullableDecl final N node;
647-
final Iterator<? extends N> childIterator;
582+
final Iterator<N> preOrder(Iterator<? extends N> startNodes) {
583+
final Deque<Iterator<? extends N>> horizon = new ArrayDeque<>();
584+
horizon.addFirst(startNodes);
585+
return new AbstractIterator<N>() {
586+
@Override
587+
protected N computeNext() {
588+
do {
589+
N next = visitNext(horizon);
590+
if (next != null) {
591+
Iterator<? extends N> successors = successorFunction.successors(next).iterator();
592+
if (successors.hasNext()) {
593+
horizon.addFirst(successors);
594+
}
595+
return next;
596+
}
597+
} while (!horizon.isEmpty());
598+
return endOfData();
599+
}
600+
};
601+
}
648602

649-
NodeAndChildren(@NullableDecl N node, Iterable<? extends N> children) {
650-
this.node = node;
651-
this.childIterator = children.iterator();
603+
final Iterator<N> postOrder(Iterator<? extends N> startNodes) {
604+
final Deque<Iterator<? extends N>> horizon = new ArrayDeque<>();
605+
horizon.addFirst(startNodes);
606+
final Deque<N> ancestorStack = new ArrayDeque<>();
607+
return new AbstractIterator<N>() {
608+
@Override
609+
protected N computeNext() {
610+
for (N next = visitNext(horizon); next != null; next = visitNext(horizon)) {
611+
Iterator<? extends N> successors = successorFunction.successors(next).iterator();
612+
if (!successors.hasNext()) {
613+
return next;
614+
}
615+
horizon.addFirst(successors);
616+
ancestorStack.push(next);
617+
}
618+
return ancestorStack.isEmpty() ? endOfData() : ancestorStack.pop();
652619
}
653-
}
620+
};
654621
}
655-
}
656622

657-
private enum Order {
658-
PREORDER,
659-
POSTORDER
623+
/**
624+
* Visits the next node from the top iterator of {@code horizon} and returns the visited node.
625+
* Null is returned to indicate reaching the end of the top iterator, which can be used by the
626+
* traversal strategies to decide what to return in such case: in pre-order, continue to poll
627+
* the next top iterator with {@code visitNext()}; in post-order, return the parent node.
628+
*
629+
* <p>For example, if horizon is {@code [[a, b], [c, d], [e]]}, {@code visitNext()} will return
630+
* {@code [a, b, null, c, d, null, e, null]} sequentially, encoding the topological structure.
631+
* (Note, however, that the callers of {@code visitNext()} often insert additional iterators
632+
* into {@code horizon} between calls to {@code visitNext()}. This causes them to receive
633+
* additional values interleaved with those shown above.)
634+
*/
635+
@NullableDecl
636+
abstract N visitNext(Deque<Iterator<? extends N>> horizon);
660637
}
661638
}

‎guava-tests/test/com/google/common/graph/TraverserTest.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -519,11 +519,11 @@ public void forGraph_depthFirstPreOrder_iterableIsLazy() {
519519
Iterable<Character> result = Traverser.forGraph(graph).depthFirstPreOrder('a');
520520

521521
assertEqualCharNodes(Iterables.limit(result, 2), "ab");
522-
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'd');
522+
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b');
523523

524524
// Iterate again to see if calculation is done again
525525
assertEqualCharNodes(Iterables.limit(result, 2), "ab");
526-
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'd', 'd');
526+
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b');
527527
}
528528

529529
@Test
@@ -532,11 +532,11 @@ public void forGraph_depthFirstPreOrderIterable_iterableIsLazy() {
532532
Iterable<Character> result = Traverser.forGraph(graph).depthFirstPreOrder(charactersOf("ac"));
533533

534534
assertEqualCharNodes(Iterables.limit(result, 2), "ab");
535-
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'c', 'd');
535+
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'b', 'c');
536536

537537
// Iterate again to see if calculation is done again
538538
assertEqualCharNodes(Iterables.limit(result, 2), "ab");
539-
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'c', 'd', 'd');
539+
assertThat(graph.requestedNodes).containsExactly('a', 'a', 'a', 'b', 'b', 'c');
540540
}
541541

542542
@Test
@@ -1022,11 +1022,11 @@ public void forTree_depthFirstPreOrder_iterableIsLazy() {
10221022
Iterable<Character> result = Traverser.forGraph(graph).depthFirstPreOrder('h');
10231023

10241024
assertEqualCharNodes(Iterables.limit(result, 2), "hd");
1025-
assertThat(graph.requestedNodes).containsExactly('h', 'h', 'd', 'a');
1025+
assertThat(graph.requestedNodes).containsExactly('h', 'h', 'd');
10261026

10271027
// Iterate again to see if calculation is done again
10281028
assertEqualCharNodes(Iterables.limit(result, 2), "hd");
1029-
assertThat(graph.requestedNodes).containsExactly('h', 'h', 'h', 'd', 'd', 'a', 'a');
1029+
assertThat(graph.requestedNodes).containsExactly('h', 'h', 'h', 'd', 'd');
10301030
}
10311031

10321032
@Test

0 commit comments

Comments
 (0)