Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edge flags refactoring #1447

Merged
merged 49 commits into from Feb 19, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
fb4045a
make measurement possible again #1351
karussell Aug 14, 2018
c43558f
refactored long flags into IntsRef
karussell Aug 15, 2018
495490b
more agressive caching
karussell Aug 17, 2018
98509d4
made core + reader-osm modules working and tests pass
karussell Aug 28, 2018
1c917c7
made gtfs compiling. Finally all tests are green. Yeah
karussell Sep 1, 2018
caa8971
removed TODO NOW, some for later some are no longer critical as all t…
karussell Sep 7, 2018
39f9b92
introduce separate parsing of shared EncodedValues like roundabout to…
karussell Sep 7, 2018
45f2de7
merged master into intsref_refactoring
karussell Sep 7, 2018
037704b
merged master into intsref_refactoring
karussell Sep 7, 2018
c3fdddf
enable to increase edge flags to more than 8 bytes
karussell Sep 10, 2018
cffad50
parse bytes for edges properly
karussell Sep 20, 2018
f0999d9
Merge branch 'master' into intsref_refactoring
karussell Sep 20, 2018
1b5b0f3
Merge branch 'master' into intsref_refactoring
karussell Oct 16, 2018
13326df
Merge branch 'master' into intsref_refactoring
karussell Oct 16, 2018
9ffcb8a
Merge remote-tracking branch 'origin/master' into intsref_refactoring
karussell Oct 16, 2018
be50349
merged master
karussell Dec 17, 2018
ab92136
JTS downgrade to 1.15.1 #1507
karussell Dec 19, 2018
d982db6
introduced interfaces for EncodedValues; replaced StringEV with EnumA…
karussell Dec 20, 2018
0590592
minor missing renames to EnumEncodedVAlue
karussell Dec 20, 2018
e8e738f
fix bug with more than 4 bytes for edge flags
karussell Dec 29, 2018
9f30377
Merge branch 'master' into intsref_refactoring
karussell Dec 29, 2018
95258df
made cachedIntsRef cleaner and reduce object creation when there are …
karussell Dec 30, 2018
9485e7e
renamed EnumAlike to IndexBased and EnumEncodedValue to ObjectEncoded…
karussell Dec 30, 2018
8e70b4e
use cached IntsRef for CH preparation
karussell Dec 30, 2018
d6b9441
trying another approach
karussell Dec 30, 2018
582aa8c
Merge branch 'master' into intsref_refactoring
karussell Jan 2, 2019
4411c16
for now make setSpeed protected
karussell Jan 3, 2019
122d7ac
merge master into intsref_refactoring
karussell Jan 3, 2019
54e70de
Merge branch 'master' into intsref_refactoring
easbar Jan 27, 2019
4feaa32
Merge branch 'master' into intsref_refactoring
easbar Feb 5, 2019
8655df4
Fixes some compile errors after merge, leaves some todos, only one te…
easbar Feb 5, 2019
26b1e21
Adjusts debugPrint() methods to changed api and intsref.
easbar Feb 5, 2019
bab2d3e
minor fix.
easbar Feb 5, 2019
0dc5b3c
Fixes EdgeBasedNodeContractorTest.
easbar Feb 6, 2019
b869aba
Merge branch 'master' into intsref_refactoring
easbar Feb 6, 2019
529a31f
Adds more efficient shortcut method (prevents iterator), but sth is w…
easbar Feb 7, 2019
e81433e
Fixes error (write flags to edges DA instead of shortcuts) and uses n…
easbar Feb 7, 2019
dc02c8f
minor
easbar Feb 7, 2019
85b2c45
minor improvements
karussell Feb 8, 2019
cb9d90f
Minor cleanup.
easbar Feb 8, 2019
8605fb3
Avoids intsref for shortcut updates and removes IntsRef creation from…
easbar Feb 8, 2019
9aa6b0d
Merge branch 'master' into intsref_refactoring
karussell Feb 13, 2019
5820cc3
removed unnecessary getDirectFlags method and some minor cleanup
karussell Feb 13, 2019
2f1e70c
shortcuts use cached chFlags but do not use setFlags
karussell Feb 13, 2019
c78624b
Removes unused intsref.
easbar Feb 15, 2019
8150979
merged origin/intsref_no_iter_cached
karussell Feb 18, 2019
c0ced26
minor cleanup
karussell Feb 18, 2019
b211a79
Merge remote-tracking branch 'origin/master' into intsref_refactoring
karussell Feb 18, 2019
52a143f
add some final
karussell Feb 18, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -19,11 +19,13 @@

import com.carrotsearch.hppc.IntHashSet;
import com.carrotsearch.hppc.IntSet;
import com.graphhopper.routing.profiles.BooleanEncodedValue;
import com.graphhopper.routing.util.DefaultEdgeFilter;
import com.graphhopper.routing.util.FlagEncoder;
import com.graphhopper.routing.weighting.TurnWeighting;
import com.graphhopper.storage.CHGraph;
import com.graphhopper.storage.GraphHopperStorage;
import com.graphhopper.storage.IntsRef;
import com.graphhopper.util.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -189,21 +191,22 @@ private void findAndHandleShortcuts(int node) {
}

private void countPreviousEdges(int node) {
BooleanEncodedValue accessEnc = encoder.getAccessEnc();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as an info: the dependency flagEncoder->EncodedValue is one that we should avoid and always prefer EncodedManager->EncodedValue if possible and simple. In this case it seems to be harder so it might be ok (that's why I created the method as a helper).

CHEdgeIterator iter = allEdgeExplorer.setBaseNode(node);
while (iter.next()) {
if (isContracted(iter.getAdjNode()))
continue;
if (iter.isForward(encoder)) {
if (iter.get(accessEnc)) {
numPrevEdges++;
}
if (iter.isBackward(encoder)) {
if (iter.getReverse(accessEnc)) {
numPrevEdges++;
}
if (!iter.isShortcut()) {
if (iter.isForward(encoder)) {
if (iter.get(accessEnc)) {
numPrevOrigEdges++;
}
if (iter.isBackward(encoder)) {
if (iter.getReverse(accessEnc)) {
numPrevOrigEdges++;
}
} else {
Expand Down Expand Up @@ -295,9 +298,12 @@ private CHEntry doAddShortcut(CHEntry edgeFrom, CHEntry edgeTo) {
LOGGER.trace("Adding shortcut from {} to {}, weight: {}, firstOrigEdge: {}, lastOrigEdge: {}",
from, adjNode, edgeTo.weight, edgeFrom.getParent().incEdge, edgeTo.incEdge);
CHEdgeIteratorState shortcut = prepareGraph.shortcut(from, adjNode);
long direction = PrepareEncoder.getScFwdDir();
int direction = PrepareEncoder.getScFwdDir();
// we need to set flags first because they overwrite weight etc
shortcut.setFlags(direction);
// TODO NOW creating a new intsref every time here is probably not efficient ?
IntsRef intsRef = new IntsRef(1);
intsRef.ints[0] = direction;
shortcut.setFlags(intsRef);
shortcut.setSkippedEdges(edgeFrom.edge, edgeTo.edge)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. As you can see in the node based contractor (addShortcuts) I'm doing the same, but I've moved the creation out of the loop so that it can be reused. Reusing is especially important if this is a hot method. IMO this is the main reason the preparation is slower compared to master.

I tried to let the Graph handle this caching and called shortcut.getFlags (creating a cached IntsRef) but as we currently do not allow to directly call getFlags for shortcuts this fails. The cleanest thing would be to create a shortcutWeightEnc and shortcutAccessEnc like we have e.g. averageSpeedEnc or accessEnc. Still in case of a new shortcut it would not be possible for the PrepareGraph to cache this (as it should be stateless), unlike when it would be a EdgeIterator or EdgeExplorer (where we assume stateful).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe you have a better idea in this regard?

Copy link
Member Author

@karussell karussell Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some possibilities:

  • allow getFlags: this avoids an additional IntsRef object in case of a new shortcut as we can just call "shortcut.getFlags". This is not that bad as getFlags is now also discouraged for edges too due to the low level usability
  • introduce accessEnc and weightEnc, which allows e.g. dynamically configuring the precision but the overhead when comparing to pure bit operations might be too high?
  • use a different API for shortcuts, e.g. fallback to long getShortcutFlags as everything that we want to store is known before and we could create setter&getter for it (like setWeight or setFirstAndLastOrigEdges). But this is inconsistent compared to normal edges.

// this is a bit of a hack, we misuse incEdge of edgeFrom's parent to store the first orig edge
.setFirstAndLastOrigEdges(edgeFrom.getParent().incEdge, edgeTo.incEdge)
Expand Down
Expand Up @@ -47,7 +47,7 @@ protected AbstractWeighting(FlagEncoder encoder) {
@Override
public long calcMillis(EdgeIteratorState edgeState, boolean reverse, int prevOrNextEdgeId) {
if (reverse && !edgeState.getReverse(accessEnc) || !reverse && !edgeState.get(accessEnc))
throw new IllegalStateException("Calculating time should not require to read speed from edge in wrong direction. "
throw new IllegalStateException("Calculating time should not require to read speed from edge in wrong direction. " +
"(" + edgeState.getBaseNode() + " - " + edgeState.getAdjNode() + ") "
+ edgeState.fetchWayGeometry(3) + " " + edgeState.getDistance() + " "
+ "Reverse:" + reverse + ", fwd:" + edgeState.get(accessEnc) + ", bwd:" + edgeState.getReverse(accessEnc));
Expand Down
Expand Up @@ -27,10 +27,7 @@
import com.graphhopper.routing.weighting.ShortestWeighting;
import com.graphhopper.routing.weighting.TurnWeighting;
import com.graphhopper.routing.weighting.Weighting;
import com.graphhopper.storage.CHGraph;
import com.graphhopper.storage.GraphBuilder;
import com.graphhopper.storage.GraphHopperStorage;
import com.graphhopper.storage.TurnCostExtension;
import com.graphhopper.storage.*;
import com.graphhopper.util.CHEdgeIteratorState;
import com.graphhopper.util.EdgeIteratorState;
import com.graphhopper.util.GHUtility;
Expand Down Expand Up @@ -633,7 +630,10 @@ private AbstractBidirectionEdgeCHNoSOD createAlgo() {
private void addShortcut(int from, int to, int firstOrigEdge, int lastOrigEdge, int skipped1, int skipped2, double weight) {
CHEdgeIteratorState shortcut = chGraph.shortcut(from, to);
// we need to set flags first because they overwrite weight etc
shortcut.setFlags(PrepareEncoder.getScFwdDir());
// TODO NOW: is this the way to go ?
IntsRef intsRef = new IntsRef(1);
intsRef.ints[0] = PrepareEncoder.getScFwdDir();
shortcut.setFlags(intsRef);
shortcut.setFirstAndLastOrigEdges(firstOrigEdge, lastOrigEdge).setSkippedEdges(skipped1, skipped2).setWeight(weight);
}

Expand Down
Expand Up @@ -20,6 +20,7 @@

import com.graphhopper.Repeat;
import com.graphhopper.RepeatRule;
import com.graphhopper.routing.profiles.BooleanEncodedValue;
import com.graphhopper.routing.util.AllCHEdgesIterator;
import com.graphhopper.routing.util.CarFlagEncoder;
import com.graphhopper.routing.util.EncodingManager;
Expand Down Expand Up @@ -1434,10 +1435,11 @@ private Set<Shortcut> getCurrentShortcuts() {
AllCHEdgesIterator iter = chGraph.getAllEdges();
while (iter.next()) {
if (iter.isShortcut()) {
BooleanEncodedValue accessEnc = encoder.getAccessEnc();
shortcuts.add(new Shortcut(
iter.getBaseNode(), iter.getAdjNode(),
iter.getOrigEdgeFirst(), iter.getOrigEdgeLast(), iter.getSkippedEdge1(), iter.getSkippedEdge2(), iter.getWeight(),
iter.isForward(encoder), iter.isBackward(encoder)
iter.get(accessEnc), iter.getReverse(accessEnc)
));
}
}
Expand Down
Expand Up @@ -25,6 +25,7 @@
import com.graphhopper.storage.CHGraph;
import com.graphhopper.storage.GraphBuilder;
import com.graphhopper.storage.GraphHopperStorage;
import com.graphhopper.storage.IntsRef;
import com.graphhopper.util.CHEdgeExplorer;
import com.graphhopper.util.CHEdgeIterator;
import com.graphhopper.util.CHEdgeIteratorState;
Expand Down Expand Up @@ -70,7 +71,10 @@ public void testAccept_fwdLoopShortcut_acceptedByInExplorer() {

private void addShortcut(CHGraph chGraph, int from, int to, boolean fwd, int firstOrigEdge, int lastOrigEdge) {
CHEdgeIteratorState shortcut = chGraph.shortcut(from, to);
shortcut.setFlags(fwd ? PrepareEncoder.getScFwdDir() : PrepareEncoder.getScBwdDir());
// TODO NOW is this the way to go ?
IntsRef intsRef = new IntsRef(1);
intsRef.ints[0] = fwd ? PrepareEncoder.getScFwdDir() : PrepareEncoder.getScBwdDir();
shortcut.setFlags(intsRef);
shortcut.setFirstAndLastOrigEdges(firstOrigEdge, lastOrigEdge);
}

Expand Down
Expand Up @@ -445,7 +445,10 @@ public void testAddShortcut_edgeBased() {
private void addShortcut(CHGraph chGraph, int from, int to, boolean fwd, int firstOrigEdge, int lastOrigEdge,
int skipEdge1, int skipEdge2, int distance) {
CHEdgeIteratorState shortcut = chGraph.shortcut(from, to);
shortcut.setFlags(fwd ? PrepareEncoder.getScFwdDir() : PrepareEncoder.getScBwdDir());
// TODO NOW is this the way to go ?
IntsRef intsRef = new IntsRef(1);
intsRef.ints[0] = fwd ? PrepareEncoder.getScFwdDir() : PrepareEncoder.getScBwdDir();
shortcut.setFlags(intsRef);
shortcut.setFirstAndLastOrigEdges(firstOrigEdge, lastOrigEdge).setSkippedEdges(skipEdge1, skipEdge2).setDistance(distance);
}

Expand Down