Skip to content

Commit

Permalink
Use iteration instead of recursion in Graphs.hasCycle.
Browse files Browse the repository at this point in the history 
This can avoid stack overflows for graphs with long paths.

RELNOTES=`graph`: Improved `Graphs.hasCycle` to avoid causing `StackOverflowError` for long paths.
PiperOrigin-RevId: 653681462
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed Jul 18, 2024
1 parent 61bfd84 commit 63734b9
Show file tree
Hide file tree
Showing 4 changed files with 158 additions and 42 deletions.
22 changes: 22 additions & 0 deletions android/guava-tests/test/com/google/common/graph/GraphPropertiesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,17 @@ public void hasCycle_multipleCycles() {
assertThat(hasCycle(undirectedGraph)).isTrue();
}

@Test
public void hasCycle_deepPathGraph() {
for (MutableGraph<Integer> graph : graphsToTest) {
for (int i = 0; i < 100000; i++) {
graph.putEdge(i, i + 1);
}
}
assertThat(hasCycle(directedNetwork)).isFalse();
assertThat(hasCycle(undirectedNetwork)).isFalse();
}

@Test
public void hasCycle_twoParallelEdges() {
for (MutableNetwork<Integer, String> network : networksToTest) {
Expand All @@ -176,4 +187,15 @@ public void hasCycle_cyclicMultigraph() {
assertThat(hasCycle(directedNetwork)).isTrue();
assertThat(hasCycle(undirectedNetwork)).isTrue();
}

@Test
public void hasCycle_deepPathNetwork() {
for (MutableNetwork<Integer, String> network : networksToTest) {
for (int i = 0; i < 100000; i++) {
network.addEdge(i, i + 1, Integer.toString(i));
}
}
assertThat(hasCycle(directedNetwork)).isFalse();
assertThat(hasCycle(undirectedNetwork)).isFalse();
}
}
78 changes: 57 additions & 21 deletions android/guava/src/com/google/common/graph/Graphs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,13 @@
import com.google.common.collect.Iterators;
import com.google.common.collect.Maps;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.ArrayDeque;
import java.util.Collection;
import java.util.Deque;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Queue;
import java.util.Set;
import javax.annotation.CheckForNull;

Expand Down Expand Up @@ -68,7 +71,7 @@ public static <N> boolean hasCycle(Graph<N> graph) {
Map<Object, NodeVisitState> visitedNodes =
Maps.newHashMapWithExpectedSize(graph.nodes().size());
for (N node : graph.nodes()) {
if (subgraphHasCycle(graph, visitedNodes, node, null)) {
if (subgraphHasCycle(graph, visitedNodes, node)) {
return true;
}
}
Expand All @@ -94,34 +97,67 @@ public static boolean hasCycle(Network<?, ?> network) {
}

/**
* Performs a traversal of the nodes reachable from {@code node}. If we ever reach a node we've
* already visited (following only outgoing edges and without reusing edges), we know there's a
* cycle in the graph.
* Performs a traversal of the nodes reachable from {@code startNode}. If we ever reach a node
* we've already visited (following only outgoing edges and without reusing edges), we know
* there's a cycle in the graph.
*/
private static <N> boolean subgraphHasCycle(
Graph<N> graph,
Map<Object, NodeVisitState> visitedNodes,
N node,
@CheckForNull N previousNode) {
NodeVisitState state = visitedNodes.get(node);
if (state == NodeVisitState.COMPLETE) {
return false;
}
if (state == NodeVisitState.PENDING) {
return true;
}
Graph<N> graph, Map<Object, NodeVisitState> visitedNodes, N startNode) {
Deque<NodeAndRemainingSuccessors<N>> stack = new ArrayDeque<>();
stack.addLast(new NodeAndRemainingSuccessors<>(startNode));

while (!stack.isEmpty()) {
// To peek at the top two items, we need to temporarily remove one.
NodeAndRemainingSuccessors<N> top = stack.removeLast();
NodeAndRemainingSuccessors<N> prev = stack.peekLast();
stack.addLast(top);

N node = top.node;
N previousNode = prev == null ? null : prev.node;
if (top.remainingSuccessors == null) {
NodeVisitState state = visitedNodes.get(node);
if (state == NodeVisitState.COMPLETE) {
stack.removeLast();
continue;
}
if (state == NodeVisitState.PENDING) {
return true;
}

visitedNodes.put(node, NodeVisitState.PENDING);
for (N nextNode : graph.successors(node)) {
if (canTraverseWithoutReusingEdge(graph, nextNode, previousNode)
&& subgraphHasCycle(graph, visitedNodes, nextNode, node)) {
return true;
visitedNodes.put(node, NodeVisitState.PENDING);
top.remainingSuccessors = new ArrayDeque<>(graph.successors(node));
}

if (!top.remainingSuccessors.isEmpty()) {
N nextNode = top.remainingSuccessors.remove();
if (canTraverseWithoutReusingEdge(graph, nextNode, previousNode)) {
stack.addLast(new NodeAndRemainingSuccessors<>(nextNode));
continue;
}
}

stack.removeLast();
visitedNodes.put(node, NodeVisitState.COMPLETE);
}
visitedNodes.put(node, NodeVisitState.COMPLETE);
return false;
}

private static final class NodeAndRemainingSuccessors<N> {
final N node;

/**
* The successors left to be visited, or {@code null} if we just added this {@code
* NodeAndRemainingSuccessors} instance to the stack. In the latter case, we'll compute the
* successors if we determine that we need them after we've performed the initial processing of
* the node.
*/
@CheckForNull Queue<N> remainingSuccessors;

NodeAndRemainingSuccessors(N node) {
this.node = node;
}
}

/**
* Determines whether an edge has already been used during traversal. In the directed case a cycle
* is always detected before reusing an edge, so no special logic is required. In the undirected
Expand Down
22 changes: 22 additions & 0 deletions guava-tests/test/com/google/common/graph/GraphPropertiesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,17 @@ public void hasCycle_multipleCycles() {
assertThat(hasCycle(undirectedGraph)).isTrue();
}

@Test
public void hasCycle_deepPathGraph() {
for (MutableGraph<Integer> graph : graphsToTest) {
for (int i = 0; i < 100000; i++) {
graph.putEdge(i, i + 1);
}
}
assertThat(hasCycle(directedNetwork)).isFalse();
assertThat(hasCycle(undirectedNetwork)).isFalse();
}

@Test
public void hasCycle_twoParallelEdges() {
for (MutableNetwork<Integer, String> network : networksToTest) {
Expand All @@ -176,4 +187,15 @@ public void hasCycle_cyclicMultigraph() {
assertThat(hasCycle(directedNetwork)).isTrue();
assertThat(hasCycle(undirectedNetwork)).isTrue();
}

@Test
public void hasCycle_deepPathNetwork() {
for (MutableNetwork<Integer, String> network : networksToTest) {
for (int i = 0; i < 100000; i++) {
network.addEdge(i, i + 1, Integer.toString(i));
}
}
assertThat(hasCycle(directedNetwork)).isFalse();
assertThat(hasCycle(undirectedNetwork)).isFalse();
}
}
78 changes: 57 additions & 21 deletions guava/src/com/google/common/graph/Graphs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@
import com.google.common.collect.Iterators;
import com.google.common.collect.Maps;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.ArrayDeque;
import java.util.Collection;
import java.util.Deque;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Optional;
import java.util.Queue;
import java.util.Set;
import javax.annotation.CheckForNull;

Expand Down Expand Up @@ -69,7 +72,7 @@ public static <N> boolean hasCycle(Graph<N> graph) {
Map<Object, NodeVisitState> visitedNodes =
Maps.newHashMapWithExpectedSize(graph.nodes().size());
for (N node : graph.nodes()) {
if (subgraphHasCycle(graph, visitedNodes, node, null)) {
if (subgraphHasCycle(graph, visitedNodes, node)) {
return true;
}
}
Expand All @@ -95,34 +98,67 @@ public static boolean hasCycle(Network<?, ?> network) {
}

/**
* Performs a traversal of the nodes reachable from {@code node}. If we ever reach a node we've
* already visited (following only outgoing edges and without reusing edges), we know there's a
* cycle in the graph.
* Performs a traversal of the nodes reachable from {@code startNode}. If we ever reach a node
* we've already visited (following only outgoing edges and without reusing edges), we know
* there's a cycle in the graph.
*/
private static <N> boolean subgraphHasCycle(
Graph<N> graph,
Map<Object, NodeVisitState> visitedNodes,
N node,
@CheckForNull N previousNode) {
NodeVisitState state = visitedNodes.get(node);
if (state == NodeVisitState.COMPLETE) {
return false;
}
if (state == NodeVisitState.PENDING) {
return true;
}
Graph<N> graph, Map<Object, NodeVisitState> visitedNodes, N startNode) {
Deque<NodeAndRemainingSuccessors<N>> stack = new ArrayDeque<>();
stack.addLast(new NodeAndRemainingSuccessors<>(startNode));

while (!stack.isEmpty()) {
// To peek at the top two items, we need to temporarily remove one.
NodeAndRemainingSuccessors<N> top = stack.removeLast();
NodeAndRemainingSuccessors<N> prev = stack.peekLast();
stack.addLast(top);

N node = top.node;
N previousNode = prev == null ? null : prev.node;
if (top.remainingSuccessors == null) {
NodeVisitState state = visitedNodes.get(node);
if (state == NodeVisitState.COMPLETE) {
stack.removeLast();
continue;
}
if (state == NodeVisitState.PENDING) {
return true;
}

visitedNodes.put(node, NodeVisitState.PENDING);
for (N nextNode : graph.successors(node)) {
if (canTraverseWithoutReusingEdge(graph, nextNode, previousNode)
&& subgraphHasCycle(graph, visitedNodes, nextNode, node)) {
return true;
visitedNodes.put(node, NodeVisitState.PENDING);
top.remainingSuccessors = new ArrayDeque<>(graph.successors(node));
}

if (!top.remainingSuccessors.isEmpty()) {
N nextNode = top.remainingSuccessors.remove();
if (canTraverseWithoutReusingEdge(graph, nextNode, previousNode)) {
stack.addLast(new NodeAndRemainingSuccessors<>(nextNode));
continue;
}
}

stack.removeLast();
visitedNodes.put(node, NodeVisitState.COMPLETE);
}
visitedNodes.put(node, NodeVisitState.COMPLETE);
return false;
}

private static final class NodeAndRemainingSuccessors<N> {
final N node;

/**
* The successors left to be visited, or {@code null} if we just added this {@code
* NodeAndRemainingSuccessors} instance to the stack. In the latter case, we'll compute the
* successors if we determine that we need them after we've performed the initial processing of
* the node.
*/
@CheckForNull Queue<N> remainingSuccessors;

NodeAndRemainingSuccessors(N node) {
this.node = node;
}
}

/**
* Determines whether an edge has already been used during traversal. In the directed case a cycle
* is always detected before reusing an edge, so no special logic is required. In the undirected
Expand Down

1 comment on commit 63734b9

@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.