(Almost) move access flag checks into weighting for CH preparation #1837

easbar · 2019-12-30T08:47:33Z

This is work towards #1835 and #1776. With this PR it should be easy to move the access flag checks that occur during CH preparation inside the weighting in a later PR.

# Conflicts: # core/src/main/java/com/graphhopper/routing/ch/CHRoutingAlgorithmFactory.java # core/src/main/java/com/graphhopper/routing/ch/PrepareCHEdgeIterator.java # core/src/main/java/com/graphhopper/routing/ch/PrepareCHEdgeIteratorImpl.java

easbar · 2019-12-30T08:49:29Z

core/src/main/java/com/graphhopper/routing/ch/PrepareCHEdgeIteratorImpl.java

+        while (true) {
+            boolean hasNext = chIterator.next();
+            if (!hasNext) {
+                return false;
+            } else if (hasAccess()) {
+                return true;
+            }
+        }


Here we are filtering edges within the iterator so we do not have to do it from the outside, the hasAccess method should ultimately only depend on the weighting

easbar · 2019-12-30T08:50:21Z

core/src/main/java/com/graphhopper/routing/ch/PrepareCHEdgeIteratorImpl.java

+
+    private boolean hasAccess() {
+        if (isShortcut()) {
+            return shortcutFilter.accept((CHEdgeIterator) chIterator);


Shortcut edges are special in that they only store a single weight and use their own kind of access flags, which will not go into weighting.

easbar · 2019-12-30T08:50:59Z

core/src/main/java/com/graphhopper/routing/ch/PrepareCHEdgeIteratorImpl.java

+            if (chIterator.getBaseNode() == chIterator.getAdjNode()) {
+                return finiteWeight(false) || finiteWeight(true);
+            }
+            return shortcutFilter.fwd && finiteWeight(false) || shortcutFilter.bwd && finiteWeight(true);


This is the essential change here: instead of checking the access flags we 'calculate' the weight

easbar · 2019-12-30T08:57:04Z

core/src/main/java/com/graphhopper/routing/ch/PrepareCHEdgeIteratorImpl.java

+        // todo: for #1776 move the access check into the weighting
+        final boolean access = reverse
+                ? chIterator.getReverse(accessEnc)
+                : chIterator.get(accessEnc);
+        if (!access) {
+            return Double.POSITIVE_INFINITY;
+        }
+        if (!needWeight) {
+            return 0;
+        }
+        return weighting.calcWeight(chIterator, reverse, EdgeIterator.NO_EDGE);


This is the only place we are calculating weights for CH preparation. The access flags are checked just before calcWeight. In a future PR it should be easy to move this inside calcWeight. There is one catch here: In many cases we do not need the weight and all we want to know is whether or not the weight is finite or not. Currently we can early exit here using the needWeight parameter. For #1776 we could use something like weighting.isFinite(iter, reverse) to cover this. For Germany, node-based I measured about 5% improvement with this check (obviously this depends on how complicated the weight calculation is). In the near future I think the edge weights will be calculated beforehand anyway, so no need to worry about this I think.

I thought about this isFinite method too, but I do not think we should introduce this. Maybe as GHUtility but not for Weighting.

If this is provided from the Weighting it is suggested we can separate soft from hard constraints, but soft constraints can always degenerate into hard constraints e.g. too big factors in multiplications leading to an overflow into infinity. And so the isFinite method has to be always like Double.isFinite(calcWeight)

We could make this clear in javadocs: isFinite would allow the creator of the weighting to benefit performance-wise but it would be wrong to 'break the contract' by returning something that does not correspond to Double.isInfinite(calcWeight). Since the performance difference (for the current FastestWeighting at least) is relatively small we can continue without the method though and it should not be a big issue to add it later when its needed.

The problem is that such a method would be hard to implement. The soft constraints (outside of Weighting.isFinite but inside calcWeight) need a very sensitive range check and so even a combination of soft constraints shall never return infinity i.e. if isFinite==false, then calcWeight<infinity should be always true.

Because if this contract is violated we'll get the problems back that we currently have due to the access and weighting separation (#367, #1234).

Or we introduce a Weighting.couldBeInfinity method that can be better used as pre-check of calcWeight and used only for performance reasons.

Or we introduce a Weighting.couldBeInfinity method that can be better used as pre-check of calcWeight and used only for performance reasons.

Yes I think this is what I meant: Be able to fully block an edge (for example we might have a boolean encoded value set to false for all highways/Autobahn for bike). I would rather call it isBlocked ?

isBlocked implies the separation that I would avoid. IMO we should call it couldBeBlocked :) ... I see a bigger problem here as the "implementor" is required to keep isBlocked and calcWeight consistent. Where the past showed that this is hard to achieve even from ourselves.

And couldBeBlocked could just use the current access bit stuff and then calcWeight can still return infinity.

But well, as you said, we probably should not introduce this right now as it is really only a performance tweak. And other (more evil) techniques like caching should be tried too ;)

easbar · 2019-12-30T08:57:59Z

core/src/main/java/com/graphhopper/routing/ch/PrepareCHGraph.java

-        return new PrepareCHEdgeIteratorImpl(chGraph.createEdgeExplorer(DefaultEdgeFilter.inEdges(encoder)), weighting);
+        return PrepareCHEdgeIteratorImpl.inEdges(chGraph.createEdgeExplorer(), weighting);


Here we got rid of in/out/allEdges and use the unfiltered edge explorer instead

easbar · 2019-12-30T09:00:50Z

core/src/main/java/com/graphhopper/util/CHEdgeIteratorState.java

+    /**
+     * @return true if this shortcut can be used in fwd direction. Do not call this method if {@link #isShortcut()} is false
+     */
+    boolean getFwdAccess();
+
+    /**
+     * @see #getFwdAccess
+     */
+    boolean getBwdAccess();


These methods are needed / used to get the access flags for shortcuts which are different to the access flags of normal edges, because they cannot be 'changed' by a configurable weighting (after the preparation has finished)

These methods are only necessary for node-based CH at the moment, because for edge-based CH the edges are assumed to be always fwd==true and bwd==false, is my understanding correct here?

Yes this is true and also pretty interesting. There are lots of shortcuts with fwd=bwd=true and this way we can store two weights with one double. The shortcuts either have the same weight for both directions or they are unidirectional, in both cases there is only one weight. For edge-based CH we do not do this optimization yet. Possibly it also will not work as well as it does for node-based because of turn costs at the skipped nodes (they usually depend on the travel direction), but of course doing this could be an improvement (50% less memory in the best (albeit very unlikely) case).

easbar · 2019-12-30T09:04:51Z

Here I stuck with the createIn/Out/AllEdgeExplorer. The alternative approach would be just using createEdgeExplorer (unfiltered) and then filter within the code that uses the iterator. I think this becomes messy because one still has to keep track of which (in/out/all) edges should be filtered. I did this up to the point where I am not using in/Out/AllExplorers to see where its going here: https://github.com/graphhopper/graphhopper/tree/filter_outside (#1839)
For now I like the approach in this PR better.

karussell

Nice!

The alternative approach would be just using createEdgeExplorer (unfiltered) and then filter within the code that uses the iterator.

Will have a look into this.

karussell · 2019-12-30T12:24:50Z

core/src/main/java/com/graphhopper/routing/ch/PrepareCHEdgeIteratorImpl.java

+        // todo: for #1776 move the access check into the weighting
+        final boolean access = reverse
+                ? chIterator.getReverse(accessEnc)
+                : chIterator.get(accessEnc);
+        if (!access) {
+            return Double.POSITIVE_INFINITY;
+        }
+        if (!needWeight) {
+            return 0;
+        }
+        return weighting.calcWeight(chIterator, reverse, EdgeIterator.NO_EDGE);


I thought about this isFinite method too, but I do not think we should introduce this. Maybe as GHUtility but not for Weighting.

If this is provided from the Weighting it is suggested we can separate soft from hard constraints, but soft constraints can always degenerate into hard constraints e.g. too big factors in multiplications leading to an overflow into infinity. And so the isFinite method has to be always like Double.isFinite(calcWeight)

karussell · 2019-12-30T12:25:12Z

core/src/main/java/com/graphhopper/util/CHEdgeIteratorState.java

+    /**
+     * @return true if this shortcut can be used in fwd direction. Do not call this method if {@link #isShortcut()} is false
+     */
+    boolean getFwdAccess();
+
+    /**
+     * @see #getFwdAccess
+     */
+    boolean getBwdAccess();


These methods are only necessary for node-based CH at the moment, because for edge-based CH the edges are assumed to be always fwd==true and bwd==false, is my understanding correct here?

karussell

👍

easbar added 10 commits December 20, 2019 11:17

Use PrepareCHEdgeExplorer/Iterator also for original edge iterators

745b2e7

Filter edges by weight instead of using access flags in PrepareCHGraph

8b25aad

Exclude loops that are blocked in both directions

69d0a9b

Try to improve performance by caching weight

aa72070

Merge branch 'master' into prepare_ch_update

531ddf7

misc

7630198

Add CHEdgeIteratorState#getFwd/BwdAccess

77f9075

Use access flags only just before weighting

49d1d7a

minor

21138b3

easbar added the refactoring label Dec 30, 2019

easbar added this to the 1.0 milestone Dec 30, 2019

easbar commented Dec 30, 2019

View reviewed changes

easbar mentioned this pull request Dec 30, 2019

Move access flag checks into Weighting #1835

Closed

karussell reviewed Dec 30, 2019

View reviewed changes

karussell approved these changes Dec 30, 2019

View reviewed changes

easbar merged commit 03dd6d4 into master Dec 30, 2019

easbar deleted the prepare_ch_update branch December 30, 2019 21:01

easbar mentioned this pull request Aug 26, 2020

Add weighted in/out edge explorers #2113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Almost) move access flag checks into weighting for CH preparation #1837

(Almost) move access flag checks into weighting for CH preparation #1837

easbar commented Dec 30, 2019

easbar Dec 30, 2019

easbar Dec 30, 2019

easbar Dec 30, 2019

easbar Dec 30, 2019

karussell Dec 30, 2019

easbar Dec 30, 2019

karussell Dec 30, 2019 •

edited

Loading

easbar Dec 30, 2019

karussell Dec 30, 2019

easbar Dec 30, 2019

easbar Dec 30, 2019

karussell Dec 30, 2019

easbar Dec 30, 2019

easbar commented Dec 30, 2019 •

edited by karussell

Loading

karussell left a comment

karussell Dec 30, 2019

karussell Dec 30, 2019

karussell left a comment

		return new PrepareCHEdgeIteratorImpl(chGraph.createEdgeExplorer(DefaultEdgeFilter.inEdges(encoder)), weighting);
		return PrepareCHEdgeIteratorImpl.inEdges(chGraph.createEdgeExplorer(), weighting);

(Almost) move access flag checks into weighting for CH preparation #1837

(Almost) move access flag checks into weighting for CH preparation #1837

Conversation

easbar commented Dec 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell Dec 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easbar commented Dec 30, 2019 • edited by karussell Loading

karussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell left a comment

Choose a reason for hiding this comment

karussell Dec 30, 2019 •

edited

Loading

easbar commented Dec 30, 2019 •

edited by karussell

Loading