Remove QueryGraph cache, fix graphhopper#1768

Author: easbar

core/src/main/java/com/graphhopper/routing/AbstractBidirectionEdgeCHNoSOD.java

@@ -47,10 +47,8 @@ public AbstractBidirectionEdgeCHNoSOD(Graph graph, TurnWeighting weighting) {
         // the inner explorers will run on the base- (or query/base-)graph edges only
         Graph baseGraph = graph.getBaseGraph();
         // we need extra edge explorers, because they get called inside a loop that already iterates over edges
-        // important: we have to use different filter ids for the edge explorers here than we use for the edge
-        // explorers in the superclass, otherwise this will not work with QueryGraph's edge explorer cache, see #1623.
-        innerInExplorer = baseGraph.createEdgeExplorer(DefaultEdgeFilter.inEdges(flagEncoder).setFilterId(1));
-        innerOutExplorer = baseGraph.createEdgeExplorer(DefaultEdgeFilter.outEdges(flagEncoder).setFilterId(1));
+        innerInExplorer = baseGraph.createEdgeExplorer(DefaultEdgeFilter.inEdges(flagEncoder));
+        innerOutExplorer = baseGraph.createEdgeExplorer(DefaultEdgeFilter.outEdges(flagEncoder));
     }
 
     @Override

core/src/main/java/com/graphhopper/routing/querygraph/QueryGraph.java

@@ -22,7 +22,10 @@
 import com.graphhopper.coll.GHIntObjectHashMap;
 import com.graphhopper.routing.util.AllEdgesIterator;
 import com.graphhopper.routing.util.EdgeFilter;
-import com.graphhopper.storage.*;
+import com.graphhopper.storage.ExtendedNodeAccess;
+import com.graphhopper.storage.Graph;
+import com.graphhopper.storage.NodeAccess;
+import com.graphhopper.storage.TurnCostExtension;
 import com.graphhopper.storage.index.QueryResult;
 import com.graphhopper.util.*;
 import com.graphhopper.util.shapes.BBox;
@@ -51,13 +54,11 @@ public class QueryGraph implements Graph {
     // todo: why do we need this and do we still need it when we stop wrapping CHGraph with QueryGraph ?
     private final QueryGraph baseGraph;
     private final TurnCostExtension wrappedExtension;
-    private final Map<EdgeFilter, EdgeExplorer> cacheMap = new HashMap<>(4);
     private final NodeAccess nodeAccess;
     private final GraphModification graphModification;
 
     // Use LinkedHashSet for predictable iteration order.
     private final Set<VirtualEdgeIteratorState> unfavoredEdges = new LinkedHashSet<>(5);
-    private boolean useEdgeExplorerCache = false;
     private final IntObjectMap<List<EdgeIteratorState>> virtualEdgesAtRealNodes;
     private final List<List<EdgeIteratorState>> virtualEdgesAtVirtualNodes;
 
@@ -93,14 +94,7 @@ private QueryGraph(Graph graph, List<QueryResult> queryResults) {
         virtualEdgesAtVirtualNodes = buildVirtualEdgesAtVirtualNodes();
 
         // create very lightweight QueryGraph which uses variables from this QueryGraph (same virtual edges)
-        baseGraph = new QueryGraph(graph.getBaseGraph(), this) {
-            // override method to avoid stackoverflow
-            @Override
-            public QueryGraph setUseEdgeExplorerCache(boolean useEECache) {
-                baseGraph.useEdgeExplorerCache = useEECache;
-                return baseGraph;
-            }
-        };
+        baseGraph = new QueryGraph(graph.getBaseGraph(), this);
     }
 
     /**
@@ -138,20 +132,6 @@ public boolean isVirtualNode(int nodeId) {
         return nodeId >= mainNodes;
     }
 
-    /**
-     * This method is an experimental feature to reduce memory and CPU resources if there are many
-     * locations ("hundreds") for one QueryGraph. EdgeExplorer instances are cached based on the {@link EdgeFilter}
-     * passed into {@link #createEdgeExplorer(EdgeFilter)}. For equal (in the java sense) {@link EdgeFilter}s always
-     * the same {@link EdgeExplorer} will be returned when caching is enabled. Care has to be taken for example for
-     * custom or threaded algorithms, when using custom {@link EdgeFilter}s, or when the same edge explorer is used
-     * with different vehicles/encoders.
-     */
-    public QueryGraph setUseEdgeExplorerCache(boolean useEECache) {
-        this.useEdgeExplorerCache = useEECache;
-        this.baseGraph.setUseEdgeExplorerCache(useEECache);
-        return this;
-    }
-
     /**
      * Set those edges at the virtual node (nodeId) to 'unfavored' that require at least a turn of
      * 100° from favoredHeading.
@@ -307,19 +287,6 @@ private int getPosOfReverseEdge(int edgeId) {
 
     @Override
     public EdgeExplorer createEdgeExplorer(final EdgeFilter edgeFilter) {
-        if (useEdgeExplorerCache) {
-            EdgeExplorer cached = cacheMap.get(edgeFilter);
-            if (cached == null) {
-                cached = createUncachedEdgeExplorer(edgeFilter);
-                cacheMap.put(edgeFilter, cached);
-            }
-            return cached;
-        } else {
-            return createUncachedEdgeExplorer(edgeFilter);
-        }
-    }
-
-    private EdgeExplorer createUncachedEdgeExplorer(final EdgeFilter edgeFilter) {
         // re-use these objects between setBaseNode calls to prevent GC
         final EdgeExplorer mainExplorer = mainGraph.createEdgeExplorer(edgeFilter);
         final VirtualEdgeIterator virtualEdgeIterator = new VirtualEdgeIterator(edgeFilter, null);

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

@@ -24,28 +24,22 @@
  * @author Peter Karich
  */
 public class DefaultEdgeFilter implements EdgeFilter {
-    private static int DEFAULT_FILTER_ID = 0;
     private final boolean bwd;
     private final boolean fwd;
     private final BooleanEncodedValue accessEnc;
-    /**
-     * Used to be able to create non-equal filter instances with equal access encoder and fwd/bwd flags.
-     */
-    private int filterId;
 
-    private DefaultEdgeFilter(BooleanEncodedValue accessEnc, boolean fwd, boolean bwd, int filterId) {
+    private DefaultEdgeFilter(BooleanEncodedValue accessEnc, boolean fwd, boolean bwd) {
         this.accessEnc = accessEnc;
         this.fwd = fwd;
         this.bwd = bwd;
-        this.filterId = filterId;
     }
 
     public static DefaultEdgeFilter outEdges(BooleanEncodedValue accessEnc) {
-        return new DefaultEdgeFilter(accessEnc, true, false, DEFAULT_FILTER_ID);
+        return new DefaultEdgeFilter(accessEnc, true, false);
     }
 
     public static DefaultEdgeFilter inEdges(BooleanEncodedValue accessEnc) {
-        return new DefaultEdgeFilter(accessEnc, false, true, DEFAULT_FILTER_ID);
+        return new DefaultEdgeFilter(accessEnc, false, true);
     }
 
     /**
@@ -54,7 +48,7 @@ public static DefaultEdgeFilter inEdges(BooleanEncodedValue accessEnc) {
      * regardless of their encoding use {@link EdgeFilter#ALL_EDGES} instead.
      */
     public static DefaultEdgeFilter allEdges(BooleanEncodedValue accessEnc) {
-        return new DefaultEdgeFilter(accessEnc, true, true, DEFAULT_FILTER_ID);
+        return new DefaultEdgeFilter(accessEnc, true, true);
     }
 
     public static DefaultEdgeFilter outEdges(FlagEncoder flagEncoder) {
@@ -69,11 +63,6 @@ public static DefaultEdgeFilter allEdges(FlagEncoder flagEncoder) {
         return DefaultEdgeFilter.allEdges(flagEncoder.getAccessEnc());
     }
 
-    public DefaultEdgeFilter setFilterId(int filterId) {
-        this.filterId = filterId;
-        return this;
-    }
-
     public BooleanEncodedValue getAccessEnc() {
         return accessEnc;
     }
@@ -104,7 +93,6 @@ public boolean equals(Object o) {
 
         if (bwd != that.bwd) return false;
         if (fwd != that.fwd) return false;
-        if (filterId != that.filterId) return false;
         return accessEnc.equals(that.accessEnc);
     }
 
@@ -113,7 +101,6 @@ public int hashCode() {
         int result = (bwd ? 1 : 0);
         result = 31 * result + (fwd ? 1 : 0);
         result = 31 * result + accessEnc.hashCode();
-        result = 31 * result + filterId;
         return result;
     }
 }

core/src/test/java/com/graphhopper/routing/ch/CHTurnCostTest.java

@@ -894,42 +894,6 @@ public void test_issue_1593_simple() {
         assertFalse("no path should be found, but found " + path.calcNodes(), path.isFound());
     }
 
-    @Test
-    public void test_issue_1623_query_graph_cache() {
-        // 4-2->5-3
-        NodeAccess na = graph.getNodeAccess();
-        na.setNode(0, 49.408550, 9.701805);
-        na.setNode(1, 49.405988, 9.706111);
-        na.setNode(2, 49.400772, 9.706245);
-        na.setNode(3, 49.403167, 9.704774);
-        na.setNode(4, 49.405817, 9.704301);
-        na.setNode(5, 49.402488, 9.707799);
-        graph.edge(2, 5, 222.771000, false);
-        graph.edge(4, 2, 583.496000, true);
-        graph.edge(3, 5, 231.495000, true);
-        graph.freeze();
-
-        RoutingAlgorithmFactory pch = automaticPrepareCH();
-        LocationIndexTree index = new LocationIndexTree(graph, new RAMDirectory());
-        index.prepareIndex();
-        QueryResult qr1 = index.findClosest(49.400772, 9.706245, EdgeFilter.ALL_EDGES);
-        QueryResult qr2 = index.findClosest(49.403167, 9.704774, EdgeFilter.ALL_EDGES);
-        QueryGraph queryGraph = QueryGraph.lookup(chGraph, qr1, qr2);
-
-        // before fixing #1623 this test only worked for a disabled edge explorer cache
-        queryGraph.setUseEdgeExplorerCache(true);
-
-        assertEquals(2, qr1.getClosestNode());
-        assertEquals(3, qr2.getClosestNode());
-        RoutingAlgorithm chAlgo = pch.createAlgo(queryGraph, AlgorithmOptions.start()
-                .traversalMode(TraversalMode.EDGE_BASED)
-                .build());
-        Path path = chAlgo.calcPath(2, 3);
-        assertTrue("no path found", path.isFound());
-        assertEquals(IntArrayList.from(2, 5, 3), path.calcNodes());
-        assertEquals(454.266, path.getDistance(), 1.e-1);
-    }
-
     @Test
     public void testRouteViaVirtualNode() {
         //   3

core/src/test/java/com/graphhopper/routing/querygraph/QueryGraphTest.java

@@ -33,7 +33,10 @@
 import org.junit.Before;
 import org.junit.Test;
 
-import java.util.*;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.List;
 
 import static com.graphhopper.storage.index.QueryResult.Position.*;
 import static com.graphhopper.util.GHUtility.updateDistancesFor;
@@ -654,70 +657,6 @@ public void testInternalAPIOriginalEdgeKey() {
                 ((VirtualEdgeIteratorState) queryGraph.getEdgeIteratorState(iter.getEdge(), 1)).getOriginalEdgeKey());
     }
 
-    @Test
-    public void useEECache() {
-        initGraph(g);
-        EdgeExplorer explorer = g.createEdgeExplorer();
-        EdgeIterator iter = explorer.setBaseNode(1);
-        assertTrue(iter.next());
-        QueryResult res = createLocationResult(2, 1.5, iter, 1, PILLAR);
-
-        QueryGraph queryGraph = lookup(res);
-        queryGraph.setUseEdgeExplorerCache(true);
-
-        EdgeExplorer edgeExplorer = queryGraph.createEdgeExplorer();
-        // using cache means same reference
-        assertSame(edgeExplorer, queryGraph.createEdgeExplorer());
-    }
-
-    @Test
-    public void useEECache_nestedLoop() {
-        //
-        // 0->3
-        // |\
-        // 1 2->6
-        //   |\
-        //   4 5
-        g.edge(0, 1, 10, false);
-        g.edge(0, 2, 10, false);
-        g.edge(0, 3, 10, false);
-        g.edge(2, 4, 10, false);
-        g.edge(2, 5, 10, false);
-        g.edge(2, 6, 10, false);
-
-        EdgeExplorer explorer = g.createEdgeExplorer();
-        EdgeIterator iter = explorer.setBaseNode(0);
-        assertTrue(iter.next());
-        QueryResult res = createLocationResult(0, 0, iter, 1, PILLAR);
-
-        QueryGraph queryGraph = lookup(Collections.singletonList(res));
-        queryGraph.setUseEdgeExplorerCache(true);
-
-        EdgeExplorer outerEdgeExplorer = queryGraph.createEdgeExplorer(DefaultEdgeFilter.outEdges(carEncoder));
-        EdgeExplorer innerEdgeExplorer = queryGraph.createEdgeExplorer(DefaultEdgeFilter.outEdges(carEncoder));
-
-        // without using filter id for DefaultEdgeFilter the filters are equal and using the explorers in a nested
-        // loop would fail
-        assertSame(outerEdgeExplorer, innerEdgeExplorer);
-
-        // using a different filter id for the second filter we get different explorers
-        outerEdgeExplorer = queryGraph.createEdgeExplorer(DefaultEdgeFilter.outEdges(carEncoder));
-        innerEdgeExplorer = queryGraph.createEdgeExplorer(DefaultEdgeFilter.outEdges(carEncoder).setFilterId(1));
-        assertNotSame(outerEdgeExplorer, innerEdgeExplorer);
-
-        // now we can safely use the two explorers in a nested loop
-        Set<String> edges = new HashSet<>();
-        EdgeIterator outerIter = outerEdgeExplorer.setBaseNode(0);
-        while (outerIter.next()) {
-            edges.add("o" + outerIter.getBaseNode() + "-" + outerIter.getAdjNode());
-            EdgeIterator innerIter = innerEdgeExplorer.setBaseNode(outerIter.getAdjNode());
-            while (innerIter.next()) {
-                edges.add("i" + innerIter.getBaseNode() + "-" + innerIter.getAdjNode());
-            }
-        }
-        assertEquals(new HashSet<>(Arrays.asList("o0-1", "o0-2", "o0-3", "i2-4", "i2-5", "i2-6")), edges);
-    }
-
     @Test
     public void testWayGeometry_edge() {
         // drawn as horizontal linear graph for simplicity