-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
MLD shortest direct path search #3788
Conversation
Yes please. I have been meaning to do this refactor for ages. ❤️
Yes,
Yeah there are two options here:
Running the complete tool-chain two times on each job seems a little overkill. Maybe we can add an own travis job that runs the MLD toolchain with the tests? There are definitely some that won't work on MLD since we haven't implemented the algorithms yet. We could tag these with |
dc84bde
to
2705911
Compare
Adjusted branch to comments and added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some algorithmic adjustment that we need to do here. Otherwise this is a great first step. 👍
.travis.yml
Outdated
@@ -207,6 +207,7 @@ script: | |||
- ./unit_tests/partition-tests | |||
- popd | |||
- npm test | |||
- node ./node_modules/cucumber/bin/cucumber.js features/ -p mld |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go into the test
target of the package.json
features/lib/osrm_loader.js
Outdated
@@ -77,7 +77,9 @@ class OSRMDirectLoader extends OSRMBaseLoader { | |||
osrmUp (callback) { | |||
if (this.osrmIsRunning()) return callback(new Error("osrm-routed already running!")); | |||
|
|||
this.child = this.scope.runBin('osrm-routed', util.format("%s -p %d", this.inputFile, this.scope.OSRM_PORT), this.scope.environment, (err) => { | |||
let command_arguments = util.format('%s -p %d', this.inputFile, this.scope.OSRM_PORT); | |||
command_arguments += this.scope.ROUTING_ALGORITHM.length > 0 ? ' --algorithm ' + this.scope.ROUTING_ALGORITHM : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just set a default for ROUTING_ALGORITHM
(e.g. CH
) and remove these conditional checks.
features/lib/osrm_loader.js
Outdated
@@ -115,7 +117,9 @@ class OSRMDatastoreLoader extends OSRMBaseLoader { | |||
osrmUp (callback) { | |||
if (this.osrmIsRunning()) return callback(); | |||
|
|||
this.child = this.scope.runBin('osrm-routed', util.format('--shared-memory=1 -p %d', this.scope.OSRM_PORT), this.scope.environment, (err) => { | |||
let command_arguments = util.format('--shared-memory=1 -p %d', this.scope.OSRM_PORT); | |||
command_arguments += this.scope.ROUTING_ALGORITHM.length > 0 ? ' --algorithm ' + this.scope.ROUTING_ALGORITHM : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, default instead of conditional.
} | ||
|
||
template <typename RandomIter, typename FacadeT> | ||
void unpackPath(const FacadeT &facade, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should be algorithm independent, right? That is why the facade was templated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unpackPath
is only needed for CH and CoreCH. Same unpacking, but different facade types
const auto weight = forward_heap.GetKey(node); | ||
|
||
auto update_upper_bounds = [&](NodeID to, EdgeWeight forward_weight, EdgeWeight edge_weight) { | ||
// Upper bound for the path source -> target with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can guarantee that all heap weights will be bigger then 0 after the initial relaxation of all heap weights with negative weight.
This would require a "pre-query" before actually start the full bi-directional dijkstra:
- Add nodes to heaps like now
- Run routeStep<FOWARD_DIRECTION> unitl forward_heap.MinKey() > 0. Now the pre-condition for a correct bi-directional Dijkstra is fulfilled.
- Run the fill bi-directional Dijkstra with termination criterion:
forward_heap.MinKey() + reverse_heap.MinKey() > upper_bound
For correctness a few other kinks in the query code need to be changed, that I will outline below.
} | ||
else | ||
// // merge edges (s,t) and (t,s) into bidirectional edge | ||
// if (forward_edge.data.weight == reverse_edge.data.weight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that some backward edges will be missing in GetAdjacentEdgeRange
cells[cell_index], weights.data(), source_boundary.data(), destination_boundary.data()}; | ||
return ConstCell{cells[cell_index], | ||
weights.data(), | ||
source_boundary.empty() ? nullptr : source_boundary.data(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what case do we need the conditional here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests that have two nodes like "a b" will have no source and boundary nodes on all levels. .data()
or referencing &..[0]
will an undefined behavior without explicit check for .empty()
static SearchEngineHeapPtr forward_heap_1; | ||
static SearchEngineHeapPtr reverse_heap_1; | ||
static SearchEngineHeapPtr forward_heap_2; | ||
static SearchEngineHeapPtr reverse_heap_2; | ||
static SearchEngineHeapPtr forward_heap_3; | ||
static SearchEngineHeapPtr reverse_heap_3; | ||
static ManyToManyHeapPtr many_to_many_heap; | ||
static MultiLayerDijkstraHeapPtr mld_forward_heap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should work towards making MLD use the same heaps that the CH variants use.
If this is generally not possible, we could also type-tag the SearchEngineData
and use the CH heap data for SeachEngineData<CH>
and the MLD versions for SearchEngineData<MLD>
.
Only algorithm dependent part that I think would really be needed would be bool HeapData::from_clique_arc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, HeapData::from_clique_arc
is only needed, but it will be 4 bytes with alignment, so i placed complete edge ID to avoid calling FindEdge
later. In CH the same flag is hidden in QueryEdge
but i don't think it is good idea to cut one bit from node ID in `EdgeBasedEdge``
@@ -23,6 +24,21 @@ struct ManyToManyHeapData : HeapData | |||
ManyToManyHeapData(NodeID p, EdgeWeight duration) : HeapData(p), duration(duration) {} | |||
}; | |||
|
|||
struct MultiLayerDijkstraHeapData : HeapData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can type-tag this if we need to have it algorithm dependent. (see below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested performance of creating a new heap and clearing, it is almost the same.
It should be changed in #3816
template <bool DIRECTION> | ||
void routingStep(const datafacade::ContiguousInternalMemoryDataFacade<algorithm::MLD> &facade, | ||
const partition::MultiLevelPartitionView &partition, | ||
const partition::CellStorageView &cells, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add an closure here that makes it easy to compute the query level for a node:
struct QueryLevel
{
const MultiLevelPartition &mlp;
const PhantomNode &source;
const PhantomNode ⌖
LevelID operator()(NodeID node)
{
auto forward_level = mlp.GetQueryLevel(source.foward_node, target.forward_node, node);
auto backward_level = mlp.GetQueryLevel(source.backward_node, target.backward_node, node);
return std::min(forward_level, backward_level);
}
};
If we can fix out partition to ensure that the level 1 cell for forward_node
and backward_node
is always the same, we can optimize to only compute one.
8582c3d
to
5dc6b7f
Compare
I have adjusted the branch to review comments, but two points still can be discussed and i have no better ideas (i pushed two additional commits into a reference branch to mld/routing-reference):
@TheMarex please could you check these two points? |
Tested locally on german-latest: customization is ~33seconds
route Munich->Hamburg ~30ms
Search space https://gist.github.com/oxidase/3015ec44f4570090a84790bba60a266d |
Questions were resolved as per slack: Above example uses the wrong query level for the backward heap. |
@TheMarex i have pushed the current state:
Still TODO: split MLD /cc @TheMarex |
👍 In that case a Algorithm-specific specialization of
Yes we need to initialize the
|
01e949b
to
5938435
Compare
@TheMarex i think i addressed all open points. Just two notes:
|
@TheMarex negative values should not be inserted into heaps or weight values after delete must be increased from negative values to 0 for starting points otherwise
This must be fixed in algorithm-independent part. One problem that I have found: 144 scenarios fail because of different snapping in 5a4ceaf. I have impression that the current snapping behavior is used implicitly: at https://github.com/oxidase/osrm-backend/blob/5a4ceaff9a47c8422f2f0c11d9bdd215b17d26d5/include/engine/guidance/assemble_steps.hpp#L227-L228 it is possible to get negative weights and durations if the first edge is changed. |
@oxidase yes good point, negative source key would still bite us. Your refactor will also allow use to address #3429 (comment) eventually I think. This would give us fixing two bugs with one fix. 😄 That code you linked only deals with the corner case of having two phantom nodes on the same segment. The routing should terminate immediately with an empty path (since source == target). Not sure how this could be influenced by these routing changes. |
Checked on Europe size:
Lisbon -> Kiev
Athens -> London
|
Full CH preprocessing takes 1415.92 seconds and query responses are
So MLD ~ 5-6 times slower than full CH |
@oxidase looks good to me know. Merge at will. 🎉 |
Rebased onto refactor/split-updater. Must be merge after #3809 |
Leaving log files opened was intentional to avoid missing output that can appear `child.on('exit',...)`. With this approach cucumber tests hit locally maximum number of opened files, because node.js keeps all log files opened.
Issue
PR solves #3759 by adding MLD
search
androuteStep
functions.Some preliminary results for germany-latest:
Query times for bayern-latest München -> Würzburg :
Here is the search space for MLD. GeoJSON annotations show level and number of visited/inserted nodes into heap, greenish is the forward heap, reddish is the reverse one.
There are some discussion points:
osrm::engine::routing_algorithms::unpackPath
is hardcoded to CH unpacking and it is not so trivial to decouple unpacking procedure. Some possibilities:annotatePath
function that will take a list of unpacked edgesdifferent member names in
EdgeData
likeQueryEdge::EdgeData::id
,EdgeBasedEdge::EdgeData::edge_id
-> it must be unified for duck-typing (turn_id
?)atm
routing_base.[ch]pp
files are CH-aware, should it be split into algorithm-awarech_routing_base.[ch]pp
,mld_routing_base.[ch]pp
and algorithm-insensitiverouting_base.[ch]pp
?no tests: would it make sense to run cucumber tests in different jobs with
-a CH
and-a MLD
or tests must be for both CH and MLD?Tasklist
Requirements / Relations