Use node weights instead of edge weights in MLD algorithm #5049

oxidase · 2018-04-23T15:21:51Z

Issue

PR implements issue #5027 and can be merged after review, but if #4876 will be merged before then the commit 061a314 won't be required.

PR removes weight and duration, forward, backward members from EdgeBasedGraphEdgeData and adds a new customizer::MultiLevelGraph that has node_weights, node_weights, is_forward_edge, is_backward_edge vectors that are used now to compute edge weights and durations as a sum of node weights and turn penalties. The size of .osrm.mldgr files for california-latest.osm.pbf changes from 382468608 to 254924800 bytes or 33% reduction.

Performance check on california-latest.osm.pbf with random A->B routes does not show regression:

In addition PR fixes two issues:

e637d69 adds an exception throw if a shared region is not found to prevent UB
cda8b1b uses the sign bit of node weight to mark oneway streets and correctly set node weight value to INVALID_EDGE_WEIGHT if the value was updated. The change must fix bugs with missing routes after traffic updates in CH algorithm.

/cc @TheMarex @freenerd

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation
review
adjust for comments
cherry pick to release branch

Requirements / Relations

#4876

oxidase · 2018-04-24T12:44:22Z

For europe-latest.osm.pbf dataset:

master
- static part has a size of 12345436828 bytes
- updateable part has a size of 18792362496 bytes
branch
- static part has a size of 12345429128 bytes
- updateable part has a size of 17417163264bytes

PR reduces the complete dataset size by 5% and the updateable part of the dataset by 7.5%:

size of input.osrm.mldgr changes from 6804054528 to 4580641280 bytes
dataset has two additional files input.osrm.end and input.osrm.enw with node durations and weights of size 422718464 bytes

/cc @TheMarex

TheMarex

Just some questions around the bit-fiddling we are doing and some nitpicks. Otherwise looks fine!

TheMarex · 2018-04-24T13:28:33Z

include/customizer/edge_based_graph.hpp

+        }
+    }
+
+    MultiLevelGraph(Vector<typename SuperT::NodeArrayEntry> node_array_,


Using some class-scope type aliases might be easier to read, like using NodeArrayEntry = typename SuperT::NodeArrayEntryT.

TheMarex · 2018-04-24T13:36:16Z

include/engine/routing_algorithms/routing_base_mld.hpp

-                BOOST_ASSERT_MSG(edge_data.weight > 0, "edge_weight invalid");
-                const EdgeWeight to_weight = weight + edge_data.weight;
+                const auto node_weight =
+                    facade.GetNodeWeight(DIRECTION == FORWARD_DIRECTION ? node : to);


Good catch, didn't think about the reverse-case here. 👍

TheMarex · 2018-04-24T13:38:06Z

include/partitioner/multi_level_graph.hpp

@@ -146,6 +149,14 @@ class MultiLevelGraph : public util::StaticGraph<EdgeDataT, Ownership>
        return max_border_node_id;
    }

+    auto data() &&


This is a kind of weird mechanism. Do you think we could also solve this by protected access to these members by the sub-class? Or is this a problem with templates?

It is not directly a problem with templates but with different EdgeBasedGraphEdgeData template arguments of customizer and partitioner graphs and customizer::MultiLevelGraph is not allowed to access protected members of partitioner::MultiLevelGraph.

An rvalue qualifier && is just a safety belt to prevent using an object instance after graph.data() call and explicit requirement for std::move(graph).data(). && here and std::move at the calling place can be removed.

TheMarex · 2018-04-24T13:38:32Z

include/storage/shared_data_index.hpp

        return region.layout.GetBlockSize(name);
    }

  private:
+    const AllocatedRegion &GetBlockRegion(const std::string &name) const


Nice refactor, thanks! 👍

TheMarex · 2018-04-24T13:40:04Z

src/contractor/contractor.cpp

+    // Convert node weights for oneway streets to INVALID_EDGE_WEIGHT
+    for (auto &weight : node_weights)
+    {
+        weight = (weight & 0x80000000) ? INVALID_EDGE_WEIGHT : weight;


I don't understand this part? We change the weight to INVALID_EDGE_WEIGHT if we MSB is set, but why not save it as INVALID_EDGE_WEIGHT in the first place?

In the extractor where the value is computed it is not possible to say how node weights will be used later:

in contraction it must be INVALID_EDGE_WEIGHT to make loop edges at

osrm-backend/src/contractor/graph_contractor.cpp

Line 193 in 3ee9495

if (path_weight < node_weights[node])

If node_weights[node] is not INVALID_EDGE_WEIGHT then path_weight of the loop edge will be always larger than the node weight value and the condition will be always false.

for MLD node_weights[node] must be a correct value otherwise GetNodeWeight will return INVALID_EDGE_WEIGHT that will break routing

The MSB flag is used to distinguish these usage scenarios.

Oh the problem is the reverse search of MLD? Okay yeah that is pretty ugly, but makes sense. I feel this deserves a better comment because this is a "hack".

TheMarex · 2018-04-24T13:43:01Z

src/customize/customizer.cpp

+    auto graph = LoadAndUpdateEdgeExpandedGraph(
+        config, mlp, node_weights, node_durations, connectivity_checksum);
+    BOOST_ASSERT(graph.GetNumberOfNodes() == node_weights.size());
+    std::for_each(node_weights.begin(), node_weights.end(), [](auto &w) { w &= 0x7fffffff; });


Same here, why can't we enforce the MSB clipping where this data is generated?

TheMarex · 2018-04-24T13:47:21Z

src/extractor/edge_based_graph_factory.cpp

    if (nbe_to_ebn_mapping[edge_id_1] != SPECIAL_NODEID &&
        nbe_to_ebn_mapping[edge_id_2] == SPECIAL_NODEID)
-        m_edge_based_node_weights[nbe_to_ebn_mapping[edge_id_1]] = INVALID_EDGE_WEIGHT;
+        m_edge_based_node_weights[nbe_to_ebn_mapping[edge_id_1]] |= 0x80000000;


I don't get what setting the MSB vs. INVALID_EDGE_WEIGHT gives us here?

TheMarex · 2018-04-27T03:48:24Z

@oxidase thanks for answering my questions. I think this might only need a better comment and then this is ready to merge.

oxidase requested a review from TheMarex April 23, 2018 15:21

TheMarex added the Review label Apr 24, 2018

TheMarex approved these changes Apr 24, 2018

View reviewed changes

TheMarex added Review - In feedback and removed Review labels Apr 24, 2018

TheMarex added Ready To Merge and removed Review - In feedback labels Apr 27, 2018

oxidase added 9 commits April 27, 2018 18:43

Throw an exception if a shared region is not found

0b71f14

List dataset blocks

c5be578

Fix for updating node weights in contraction hierarchy

6fadc5b

Add customizer::MultiLevelGraph

a00ee71

Always read .osrm.enw file in updater

579ea7b

Add EBG node durations

d4e97d0

Remove weight and duration from customizer::EdgeBasedGraphEdgeData

5be284e

Don't handle response errors separately in osrm-runner

4773dd7

Review and rebase updates

557c3e6

oxidase force-pushed the mld/node-weights branch from 3ee9495 to 557c3e6 Compare April 27, 2018 16:57

TheMarex merged commit cacb162 into master Apr 28, 2018

TheMarex deleted the mld/node-weights branch April 28, 2018 06:59

oxidase mentioned this pull request Apr 30, 2018

Adjust ebn_weight assertion to new node weights #5061

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use node weights instead of edge weights in MLD algorithm #5049

Use node weights instead of edge weights in MLD algorithm #5049

oxidase commented Apr 23, 2018

oxidase commented Apr 24, 2018

TheMarex left a comment

TheMarex Apr 24, 2018

TheMarex Apr 24, 2018

TheMarex Apr 24, 2018

oxidase Apr 24, 2018

TheMarex Apr 24, 2018

TheMarex Apr 24, 2018

oxidase Apr 24, 2018

TheMarex Apr 27, 2018

TheMarex Apr 24, 2018

TheMarex Apr 24, 2018

TheMarex commented Apr 27, 2018

Use node weights instead of edge weights in MLD algorithm #5049

Use node weights instead of edge weights in MLD algorithm #5049

Conversation

oxidase commented Apr 23, 2018

Issue

Tasklist

Requirements / Relations

oxidase commented Apr 24, 2018

TheMarex left a 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

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

TheMarex commented Apr 27, 2018