Implemented Tarjan's algorithm for removing dead-end nodes.

 - Previous two pass algorithm didn't work in all cases.
 - Tarjan's is a single pass, and ~2x the speed.

Author: rodo-r2r

core/src/main/java/com/graphhopper/routing/util/PrepareRoutingSubnetworks.java

@@ -27,9 +27,12 @@
 import java.util.*;
 import java.util.Map.Entry;
 import java.util.concurrent.atomic.AtomicInteger;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import gnu.trove.list.array.TIntArrayList;
+
 /**
  * Removes nodes which are not part of the largest network. Ie. mostly nodes with no edges at all
  * but also small subnetworks which are nearly always bugs in OSM data or indicate otherwise
@@ -146,7 +149,7 @@ protected final boolean checkAdjacent( EdgeIteratorState iter )
     }
 
     /**
-     * Deletes all but the larges subnetworks.
+     * Deletes all but the largest subnetworks.
      */
     void keepLargeNetworks( Map<Integer, Integer> map )
     {
@@ -231,86 +234,29 @@ int removeZeroDegreeNodes()
 
     /**
      * Clean small networks that will be never be visited by this explorer See #86 For example,
-     * small areas like parkings are sometimes connected to the whole network through one-way road
+     * small areas like parking lots are sometimes connected to the whole network through a one-way road.
      * This is clearly an error - but is causes the routing to fail when point get connected to this
-     * small area This routines removed all these points from the graph The algorithm is to through
-     * the graph, build the network map and for each small map remove the network
+     * small area. This routines removed all these points from the graph.
      * <p/>
-     * @return removed nodes;
+     * @return number of removed nodes;
      */
     public int removeDeadEndUnvisitedNetworks( final FlagEncoder encoder )
     {
-        int removed = 0;
-        removed += removeDeadEndUnvisitedNetworks(g.createEdgeExplorer(new DefaultEdgeFilter(encoder, true, false)));
-        removed += removeDeadEndUnvisitedNetworks(g.createEdgeExplorer(new DefaultEdgeFilter(encoder, false, true)));
-        return removed;
-    }
-
-    private static <K, V extends Comparable<V>> Map<K, V> sortByValues( final Map<K, V> map )
-    {
-        Comparator<K> valueComparator = new Comparator<K>()
-        {
-            @Override
-            public int compare( K k1, K k2 )
-            {
-                int compare = map.get(k2).compareTo(map.get(k1));
-                if (compare == 0)
-                    return 1;
-                else
-                    return compare;
-            }
-        };
-        Map<K, V> sortedByValues = new TreeMap<K, V>(valueComparator);
-        sortedByValues.putAll(map);
-        return sortedByValues;
-    }
-
-    public int removeDeadEndUnvisitedNetworks( final EdgeExplorer explorer )
-    {
-        final AtomicInteger removed = new AtomicInteger(0);
-
-        // Find subnetworks according to this explorer
-        // Sort the map by largest networks first
-        Map<Integer, Integer> map = sortByValues(findSubnetworks(explorer));
-        if (map.size() < 2)
-            return 0;
-
-        //  big networks will populate bs first so these nodes won't be deleted
-        final GHBitSetImpl bs = new GHBitSetImpl(g.getNodes());
-        boolean first = true;
-        for (Entry<Integer, Integer> e : map.entrySet())
-        {
-            int mapStart = e.getKey();
-            int subnetSize = e.getValue();
-            final boolean removeNetwork = !first && (subnetSize < minOnewayNetworkSize);
-            if (first)
-                first = false;
-
-            if (removeNetwork)
-                logger.debug("Removing dead-end network: " + subnetSize + " nodes starting from nodeid=" + mapStart);
+        // Partition g into strongly connected components using Tarjan's Algorithm.
+        final EdgeFilter filter = new DefaultEdgeFilter(encoder, false, true);
+        List<TIntArrayList> components = new TarjansStronglyConnectedComponentsAlgorithm(g, filter).findComponents();
 
-            new XFirstSearch()
-            {
-                @Override
-                protected GHBitSet createBitSet()
-                {
-                    return bs;
-                }
+        // remove components less than minimum size
+        int removed = 0;
+        for (TIntArrayList component : components) {
 
-                @Override
-                protected final boolean goFurther( int nodeId )
-                {
-                    if (removeNetwork)
-                    {
-                        // This remaining node is member of a small disconnected network
-                        g.markNodeRemoved(nodeId);
-                        removed.incrementAndGet();
-                    }
-                    return super.goFurther(nodeId);
+            if (component.size() < minOnewayNetworkSize) {
+                for (int i = 0; i < component.size(); i++) {
+                    g.markNodeRemoved(component.get(i));
+                    removed ++;
                 }
-            }.start(explorer, mapStart, false);
+            }
         }
-
-        return removed.get();
+        return removed;
     }
 }

core/src/main/java/com/graphhopper/routing/util/TarjansStronglyConnectedComponentsAlgorithm.java

@@ -0,0 +1,144 @@
+package com.graphhopper.routing.util;
+
+import com.graphhopper.coll.GHBitSetImpl;
+import com.graphhopper.storage.GraphStorage;
+import com.graphhopper.util.EdgeIterator;
+import gnu.trove.list.array.TIntArrayList;
+import gnu.trove.stack.array.TIntArrayStack;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Stack;
+
+/**
+ * Implementation of Tarjan's algorithm using an explicit stack.
+ * (The traditional recursive approach runs into stack overflow pretty quickly.)
+ *
+ * Used for finding strongly strongly connected components to detect dead-ends.
+ *
+ * http://en.wikipedia.org/wiki/Tarjan's_strongly_connected_components_algorithm
+ */
+public class TarjansStronglyConnectedComponentsAlgorithm {
+
+    private final GraphStorage g;
+    private final TIntArrayStack nodeStack;
+    private final GHBitSetImpl onStack;
+    private final int[] nodeIndex;
+    private final int[] nodeLowLink;
+    private final ArrayList<TIntArrayList> components = new ArrayList<TIntArrayList>();
+
+    private int index = 1;
+    private final EdgeFilter edgeFilter;
+
+    public TarjansStronglyConnectedComponentsAlgorithm(final GraphStorage g, final EdgeFilter edgeFilter) {
+        this.g = g;
+        this.nodeStack = new TIntArrayStack();
+        this.onStack = new GHBitSetImpl(g.getNodes());
+        this.nodeIndex = new int[g.getNodes()];
+        this.nodeLowLink = new int[g.getNodes()];
+        this.edgeFilter = edgeFilter;
+    }
+
+    /**
+     * Find and return list of all strongly connected components in g.
+     */
+    public List<TIntArrayList> findComponents() {
+
+        int nodes = g.getNodes();
+        for (int start = 0; start < nodes; start++) {
+            if (nodeIndex[start] == 0 && !g.isNodeRemoved(start)) {
+                strongConnect(start);
+            }
+        }
+
+        return components;
+    }
+
+    // Find all components reachable from firstNode, add them to 'components'
+    private void strongConnect(int firstNode) {
+        final Stack<TarjanState> stateStack = new Stack<TarjanState>();
+        stateStack.push(TarjanState.startState(firstNode));
+
+        // nextState label is equivalent to the function entry point in the recursive Tarjan's algorithm.
+        nextState:
+
+        while (!stateStack.empty()) {
+            TarjanState state = stateStack.pop();
+            final int start = state.start;
+            final EdgeIterator iter;
+
+            if (state.isStart()) {
+                // We're traversing a new node 'start'.  Set the depth index for this node to the smallest unused index.
+                nodeIndex[start] = index;
+                nodeLowLink[start] = index;
+                index ++;
+                nodeStack.push(start);
+                onStack.set(start);
+
+                iter = g.createEdgeExplorer(edgeFilter).setBaseNode(start);
+
+            } else { // if (state.isResume()) {
+
+                // We're resuming iteration over the next child of 'start', set lowLink as appropriate.
+                iter = state.iter;
+
+                int prevConnectedId = iter.getAdjNode();
+                nodeLowLink[start] = Math.min(nodeLowLink[start], nodeLowLink[prevConnectedId]);
+            }
+
+            // Each element (excluding the first) in the current component should be able to find
+            // a successor with a lower nodeLowLink.
+            while (iter.next())
+            {
+                int connectedId = iter.getAdjNode();
+                if (nodeIndex[connectedId] == 0) {
+                    // Push resume and start states onto state stack to continue our DFS through the graph after the jump.
+                    // Ideally we'd just call strongConnectIterative(connectedId);
+                    stateStack.push(TarjanState.resumeState(start, iter));
+                    stateStack.push(TarjanState.startState(connectedId));
+                    continue nextState;
+                } else if (onStack.contains(connectedId)) {
+                    nodeLowLink[start] = Math.min(nodeLowLink[start], nodeIndex[connectedId]);
+                }
+            }
+
+            // If nodeLowLink == nodeIndex, then we are the first element in a component.
+            // Add all nodes higher up on nodeStack to this component.
+            if (nodeIndex[start] == nodeLowLink[start]) {
+                TIntArrayList component = new TIntArrayList();
+                int node;
+                while ((node = nodeStack.pop()) != start) {
+                    component.add(node);
+                    onStack.clear(node);
+                }
+                component.add(start);
+                onStack.clear(start);
+
+                components.add(component);
+            }
+        }
+    }
+
+    // Internal stack state of algorithm, used to avoid recursive function calls and hitting stack overflow exceptions.
+    // State is either 'start' for new nodes or 'resume' for partially traversed nodes.
+    private static class TarjanState {
+        final int start;
+        final EdgeIterator iter;
+
+        // Iterator only present in 'resume' state.
+        boolean isStart() { return iter == null; }
+
+        private TarjanState(final int start, final EdgeIterator iter) {
+            this.start = start;
+            this.iter = iter;
+        }
+
+        public static TarjanState startState(int start) {
+            return new TarjanState(start, null);
+        }
+
+        public static TarjanState resumeState(int start, EdgeIterator iter) {
+            return new TarjanState(start, iter);
+        }
+    }
+}

core/src/test/java/com/graphhopper/routing/util/PrepareRoutingSubnetworksTest.java

@@ -21,11 +21,13 @@
 import com.graphhopper.storage.GraphStorage;
 import com.graphhopper.util.EdgeExplorer;
 import com.graphhopper.util.GHUtility;
+
+import gnu.trove.list.array.TIntArrayList;
 import java.util.Arrays;
+import java.util.List;
 import java.util.Map;
 import org.junit.*;
 import static org.junit.Assert.*;
-
 /**
  *
  * @author Peter Karich
@@ -162,21 +164,31 @@ GraphStorage createDeadEndUnvisitedNetworkGraph( EncodingManager em )
         g.edge(7, 8, 1, false);
         g.edge(8, 9, 1, true);
         g.edge(9, 10, 1, true);
+
         return g;
     }
 
-    @Test
-    public void testRemoveDeadEndUnvisitedNetworksOneWay()
+    GraphStorage createTarjanTestGraph()
     {
-        GraphStorage g = createDeadEndUnvisitedNetworkGraph(em);
-        PrepareRoutingSubnetworks instance = new PrepareRoutingSubnetworks(g, em).setMinOnewayNetworkSize(3);
-        FlagEncoder encoder = em.getSingle();
-        EdgeExplorer explorer = g.createEdgeExplorer(new DefaultEdgeFilter(encoder, false, true));
-        int removed = instance.removeDeadEndUnvisitedNetworks(explorer);
-        assertEquals(2, removed);
+        GraphStorage g = createGraph(em);
 
-        g.optimize();
-        assertEquals(9, g.getNodes());
+        g.edge(1, 2, 1, false);
+        g.edge(2, 3, 1, false);
+        g.edge(3, 1, 1, false);
+
+        g.edge(4, 2, 1, false);
+        g.edge(4, 3, 1, false);
+        g.edge(4, 5, 1, true);
+        g.edge(5, 6, 1, false);
+
+        g.edge(6, 3, 1, false);
+        g.edge(6, 7, 1, true);
+
+        g.edge(8, 5, 1, false);
+        g.edge(8, 7, 1, false);
+        g.edge(8, 8, 1, false);
+
+        return g;
     }
 
     @Test
@@ -193,4 +205,39 @@ public void testRemoveDeadEndUnvisitedNetworks()
         g.optimize();
         assertEquals(8, g.getNodes());
     }
+
+    @Test
+    public void testTarjan()
+    {
+        GraphStorage g = createSubnetworkTestGraph();
+
+        // Requires a single vehicle type, otherwise we throw.
+        final FlagEncoder flagEncoder = em.getSingle();
+        final EdgeFilter filter = new DefaultEdgeFilter(flagEncoder, false, true);
+
+        TarjansStronglyConnectedComponentsAlgorithm tarjan = new TarjansStronglyConnectedComponentsAlgorithm(g, filter);
+
+        List<TIntArrayList> components = tarjan.findComponents();
+
+        assertEquals(4, components.size());
+        assertEquals(new TIntArrayList(new int[]{ 13, 5, 3, 7, 0 }), components.get(0));
+        assertEquals(new TIntArrayList(new int[]{ 2, 4, 12, 11, 8, 1 }), components.get(1));
+        assertEquals(new TIntArrayList(new int[] {10, 14, 6}), components.get(2));
+        assertEquals(new TIntArrayList(new int[] {9}), components.get(3));
+    }
+
+    // Previous two-pass implementation failed on 1 -> 2 -> 0
+    @Test
+    public void testNodeOrderingRegression() {
+        // 1 -> 2 -> 0
+        GraphStorage g = createGraph(em);
+        g.edge(1, 2, 1, false);
+        g.edge(2, 0, 1, false);
+
+        PrepareRoutingSubnetworks instance = new PrepareRoutingSubnetworks(g, em).setMinOnewayNetworkSize(2);
+        int removed = instance.removeDeadEndUnvisitedNetworks(em.getSingle());
+        
+        assertEquals(3, removed);
+    }
+
 }