Via way restrictions #4255

MoKob · 2017-07-07T14:00:21Z

Issue

refactors turn restrictions to follow strict types/structures.
adds support for via-way turn restrictions
first step towards Turn-Restrictions with via ways #2681

Tasklist

add regression / cucumber cases (see docs/testing.md)
add changelog entry indicating breaking changes to data-files
review
adjust for comments

Performance:

Extraction of Germany:

	Command being timed: "./build/osrm-extract /data/europe/germany/germany-latest.osm.pbf"
	User time (seconds): 1820.21
	System time (seconds): 120.63
	Percent of CPU this job got: 289%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 11:09.29
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 29249273856
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 558
	Minor (reclaiming a frame) page faults: 7944112
	Voluntary context switches: 1733
	Involuntary context switches: 6452780

Contraction:

	Command being timed: "./build/osrm-contract /data/europe/germany/germany-latest.osrm"
	User time (seconds): 5743.62
	System time (seconds): 61.59
	Percent of CPU this job got: 350%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 27:37.11
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 12026462208
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 237
	Minor (reclaiming a frame) page faults: 2390626
	Voluntary context switches: 2106
	Involuntary context switches: 3609162

Partition

[info] RAM: peak bytes used: 2839437312
	Command being timed: "./build/osrm-partition /data/europe/germany/germany-latest.osrm"
	User time (seconds): 2416.88
	System time (seconds): 48.81
	Percent of CPU this job got: 312%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 13:09.87
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 11357749248
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 133
	Minor (reclaiming a frame) page faults: 3211197
	Voluntary context switches: 33
	Involuntary context switches: 2509986

Customize

	Command being timed: "./build/osrm-customize /data/europe/germany/germany-latest.osrm"
	User time (seconds): 92.72
	System time (seconds): 4.02
	Percent of CPU this job got: 267%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:36.15
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 11178278912
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 153
	Minor (reclaiming a frame) page faults: 1078677
	Voluntary context switches: 18
	Involuntary context switches: 68336

Master

Extraction

	Command being timed: "./build/osrm-extract /data/europe/germany/germany-latest.osm.pbf"
	User time (seconds): 1803.88
	System time (seconds): 127.52
	Percent of CPU this job got: 285%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 11:16.82
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 29302489088
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 525
	Minor (reclaiming a frame) page faults: 9716627
	Voluntary context switches: 2248
	Involuntary context switches: 4430620

Contraction

	Command being timed: "./build/osrm-contract /data/europe/germany/germany-latest.osrm"
	User time (seconds): 5772.61
	System time (seconds): 68.48
	Percent of CPU this job got: 353%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 27:31.05
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 12026134528
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 232
	Minor (reclaiming a frame) page faults: 2396365
	Voluntary context switches: 3366
	Involuntary context switches: 4783880

Partition

	Command being timed: "./build/osrm-partition /data/europe/germany/germany-latest.osrm"
	User time (seconds): 2257.08
	System time (seconds): 46.40
	Percent of CPU this job got: 332%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 11:32.09
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 11366449152
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 133
	Minor (reclaiming a frame) page faults: 3355731
	Voluntary context switches: 33
	Involuntary context switches: 1645742

Customize

	Command being timed: "./build/osrm-customize /data/europe/germany/germany-latest.osrm"
	User time (seconds): 98.63
	System time (seconds): 5.48
	Percent of CPU this job got: 242%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:42.99
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 11177803776
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 155
	Minor (reclaiming a frame) page faults: 1078662
	Voluntary context switches: 15
	Involuntary context switches: 172589

daniel-j-h · 2017-07-10T10:36:28Z

features/car/restrictions.feature

+            | ef    |
+
+        And the relations
+            | type        | way:from | way:via | way:from | restriction   |


Here and below: shouldn't it be way:to instead of way:from twice?

daniel-j-h · 2017-07-10T10:42:06Z

include/extractor/restriction.hpp

-        from.node = SPECIAL_NODEID;
-        to.node = SPECIAL_NODEID;
+        return flags.way_type ? boost::get<InputWayRestriction>(node_or_way).from
+                              : boost::get<InputNodeRestriction>(node_or_way).from;


I don't understand this construct - above we're using a variant but we're also storing flags? The variant already contains a flag and knows what actual type it is - I think you can use a visitor or a checked get call here.

daniel-j-h · 2017-07-10T10:44:00Z

include/extractor/restriction.hpp

-    using value_type = InputRestrictionContainer;
-    bool operator()(const InputRestrictionContainer &a, const InputRestrictionContainer &b) const
+    boost::variant<NodeRestriction, WayRestriction> node_or_way;
+    restriction_details::Bits flags;


Same question here

This is an open change I have to do. I was originally using the normal structs that were in the data and only then switched to a true variant to avoid reinterpreting data over multiple types. Need to update that.

MoKob · 2017-07-13T13:22:10Z

Demo Server:

This PR:

Relation in question: https://www.openstreetmap.org/relation/6495959#map=19/37.77034/-122.51128

MoKob · 2017-07-18T15:23:00Z

Impact so far on Baden Wuerttemberg:

extraction stays more or less the same speed:

without:
real 1m36.738s
user 4m12.325s
sys 0m17.995s

with:
real 1m35.268s
user 4m18.018s
sys 0m17.866s

Contraction measurements fluctuate quite a bit but do not show a consistent slowdown over current master branch.

daniel-j-h

left some early comments, didn't make it through completely today - will follow up tomorrow

daniel-j-h · 2017-07-25T13:26:41Z

features/car/restrictions.feature

+
+        And the relations
+            | type        | way:from | node:via | way:to | restriction   |
+            | restriction | abc      | c        | cgh    | no_right_turn |


What happens if the via node is placed on a node that gets compressed? This should only ever be possible on degree-two intersections and then a restriction does not really make sense, no? What would happen in this case?

We should just ignore them. I would consider these a mapping error to begin with and not worry about them.
Internally

a - b - c and ab via b to bc should turn out as ac via b to ac after compression and be ignored as inconsistent.

daniel-j-h · 2017-07-25T13:29:20Z

features/car/restrictions.feature

+            a - b - c - - - - - - - - - - - - - - - - - - - f
+                |   | \                                     /
+                i - d - e - - - - - - - - - - - - - - - - -
+            """


Why the long geometry here?

The correct turn needs to be undesirable, even without the turn restrictions. Otherwise having multiple non-working restrictions could be hidden by the fact that the non-restricted path is better, based on turn penalties alone.

daniel-j-h · 2017-07-25T13:31:25Z

features/car/restrictions.feature

+        When I route I should get
+            | from | to | route                  |
+            | a    | j  | left,first,right,right |
+            | f    | e  | right,third,left,left  |


daniel-j-h · 2017-07-25T13:35:33Z

features/car/restrictions.feature

+
+        And the relations
+            | type        | way:from | way:via | way:to | restriction  |
+            | restriction | ab       | be      | ef     | only_left_on |


Hm is the behavior the same when I place a only_straight_on for the turn onto ef?

I am not sure whether I follow, but yes. We evaluate only_* without checking the postfix. Just as we did for via-nodes.

daniel-j-h · 2017-07-25T13:35:47Z

features/car/restrictions.feature

+
+        And the relations
+            | type        | way:from | way:via | way:to | restriction   |
+            | restriction | ab       | be      | ed     | only_right_on |


Same question here

daniel-j-h · 2017-07-25T13:48:28Z

include/extractor/edge_based_graph_factory.hpp

-             const std::string &cnbg_ebg_mapping_path);
+             const std::string &cnbg_ebg_mapping_path,
+             const RestrictionMap &restriction_map,
+             const WayRestrictionMap &way_restriction_map);


Should we name the RestrictionMap into NodeRestrictionMap then?

daniel-j-h · 2017-07-25T13:52:17Z

include/extractor/edge_based_graph_factory.hpp

+
+    // the number of edge-based nodes is mostly made up out of the edges in the node-based graph.
+    // Any edge in the node-based graph represents a node in the edge-based graph. In addition, we
+    // add a set of artificial edge-based nodes into the mix to model via-way turn restrictions.


Could you (capitalize the T :P and) add the ticket where we scoped out the idea behind via-way restrictions please

daniel-j-h · 2017-07-25T13:53:50Z

include/extractor/edge_based_graph_factory.hpp

@@ -152,14 +157,16 @@ class EdgeBasedGraphFactory

    unsigned RenumberEdges();

-    std::vector<NBGToEBG> GenerateEdgeExpandedNodes();
+    std::vector<NBGToEBG> GenerateEdgeExpandedNodes(const WayRestrictionMap &way_restriction_map);


What is the semantic meaning behind passing the way restriction map to GenerateEdgeExpandedNodes? Because it has to be aware of the artificial nodes? The idea here is not clear from the interface alone.

Will document here. You are correct. EdgeExpandedNodes has to generate all our duplicated nodes as well. Therefor it requires access to the way-restriction-map.

daniel-j-h · 2017-07-25T13:54:30Z

include/extractor/edge_based_graph_factory.hpp

-                                   const std::string &turn_penalties_index_filename);
+                                   const std::string &turn_penalties_index_filename,
+                                   const RestrictionMap &restriction_map,
+                                   const WayRestrictionMap &way_restriction_map);


Same here - why do we need the maps for edge expanded edge generation? Should this be refactored?

Before they were stored as members and used within. We require the maps to check which turns are allowed. The intersection-generation / handling of via-way restrictions is required when looking at the edge-based-edges to find out which of the turns are valid (entry_allowed flag).

I just moved this to a reference parameter to get rid of some pointer usage we did beforehand.

daniel-j-h · 2017-07-25T14:00:35Z

include/extractor/extraction_containers.hpp

    unsigned max_internal_node_id;
+
+    // list of restrictions before we transform them into the output types
+    std::vector<InputConditionalTurnRestriction> restrictions_list;


With output types you mean the turn restriction vectors below? Why do we need to store both input and output then?

Input restrictions are generated during parsing and store OSM IDs. Only after we performed our re-mapping, we can assign the actual restrictions to be used later on.

TheMarex

Most of the comments are duplicates that target the usage of lambda functions. The yoda-style (how before the what) of declaring them first and referencing them later makes it hard for me to follow the control flow, especially while reviewing, which is super frustrating. Please keep this in mind the future.

The approach is sound! I would be interested in some empirical measurements of graph size increase, just so we know what we are dealing here with. 👍

Q.A. wise I would love to see some sanity checks on California w.r.t to processing time, just to make sure we will not see unexpected results.

TheMarex · 2017-07-24T17:01:33Z

features/car/restrictions.feature

+
+        When I route I should get
+            | from | to | route             |
+            | a    | d  | ab,be,ef,ef,de,de |


Sometimes it's helpful to include an extra comment column to explain what each row is testing. For example does not use turn ab,be,de or something like that.

TheMarex · 2017-07-24T17:05:34Z

include/extractor/restriction.hpp

@@ -5,111 +5,274 @@
 #include "util/opening_hours.hpp"
 #include "util/typedefs.hpp"

+#include <boost/variant.hpp>


Is there any particular reason why the boost version is used here? We already bundle the mapbox/variant, it might make sense to use it here as well.

TheMarex · 2017-07-24T17:12:20Z

include/extractor/restriction.hpp

+        return from != SPECIAL_NODEID && to != SPECIAL_NODEID && via != SPECIAL_NODEID;
+    };
+
+    std::string ToString() const


What about moving this debug function outside of the class? I think we have an header for enabling pretty printers like this.

TheMarex · 2017-07-24T17:17:01Z

include/extractor/restriction.hpp

+        }
+    }
+
+    std::string ToString() const


Same here, pretty printers for debugging should be outside of the class.

TheMarex · 2017-07-24T17:18:41Z

include/extractor/restriction_compressor.hpp

+    // contracted move the head pointer to their respective head. Edges starting at tail move the
+    // tail values to their respective tails. Way turn restrictions are represented by two
+    // node-restrictions, so we can focus on them alone
+    boost::unordered_multimap<NodeID, NodeRestriction *> heads;


std also has a multimap, no need for boost.

~~https://stackoverflow.com/questions/11519648/unordered-multimapequal-range-slow~~

Actually, that link is a wrong one. But @daniel-j-h, you had some links indicating performance problems on std::unordered_map, care to share them here?

I think it was https://bugs.llvm.org/show_bug.cgi?id=21275

TheMarex · 2017-07-25T20:31:14Z

src/extractor/way_restriction_map.cpp

+    };
+    std::for_each(turn_restrictions.begin(), turn_restrictions.end(), extract_restrictions);
+
+    const auto as_duplicated_node =


If this function gets inlined into by_duplicated_node we could simply use std::tie(way.in_restriction.via, way.out_restriction.via, way.in_restriction.from) and avoid the copy over the tuple.

TheMarex · 2017-07-25T20:33:28Z

src/extractor/way_restriction_map.cpp

+            std::make_pair(std::make_pair(way.in_restriction.from, way.in_restriction.via), index));
+        ++index;
+    };
+    std::for_each(restriction_data.begin(), restriction_data.end(), prepare_way_restriction);


This should be a for loop for sure as it keep external state. Also as_duplicate_node is bound but not used, and I don't see a reference to duplication_id.

TheMarex · 2017-07-25T20:35:21Z

src/extractor/way_restriction_map.cpp

+        ++offset;
+        return false; // continue until the end
+    };
+    std::adjacent_find(restriction_data.begin(), restriction_data.end(), add_offset_on_new_groups);


We have util::for_each_pair for this, which makes the intend a little bit clearer.

TheMarex · 2017-07-25T20:36:22Z

src/extractor/way_restriction_map.cpp

+std::size_t WayRestrictionMap::AsDuplicatedNodeID(const std::size_t restriction_id) const
+{
+    return std::distance(duplicated_node_groups.begin(),
+                         std::upper_bound(duplicated_node_groups.begin(),


This should be split to two expression for readability, the 1 is almost sneaking away at the end.

TheMarex · 2017-07-25T20:37:27Z

src/extractor/way_restriction_map.cpp

+{
+    // loop over all restrictions associated with the node. Mark as restricted based on
+    // is_only/restricted targets
+    for (std::size_t restriction_index = duplicated_node_groups[duplicated_node];


util::irange can be used for this.

Makes turn restrictions into dedicated structures and diferentiates between them via a variant. Ensures that we do not accidentally mess up ID types within our application. In addition this improves the restriction performance by only parsing all edges once at the cost of (at the time of writing) 22MB in terms of main memory usage.

…eation of edge-based-graph initial version of handling via-way turn restrictions (this is dirty) - requires update of data structures - requires clean-up - requires optimisation

MoKob · 2017-07-28T12:40:00Z

I've addressed the reviews. Could you re-check?

MoKob added the Work In Progress label Jul 7, 2017

MoKob force-pushed the via-way-restrictions branch from cf5f718 to 9dc3c15 Compare July 7, 2017 14:02

daniel-j-h self-requested a review July 7, 2017 15:08

MoKob force-pushed the via-way-restrictions branch from 9dc3c15 to 177f0fc Compare July 7, 2017 15:23

daniel-j-h requested changes Jul 10, 2017

View reviewed changes

MoKob force-pushed the via-way-restrictions branch 5 times, most recently from 58946c6 to 07459e9 Compare July 13, 2017 10:21

MoKob force-pushed the via-way-restrictions branch 3 times, most recently from 012c2f8 to 29e0cc7 Compare July 18, 2017 12:52

MoKob force-pushed the via-way-restrictions branch 3 times, most recently from 27cd3c7 to 6109840 Compare July 19, 2017 13:18

MoKob added Review and removed Work In Progress labels Jul 19, 2017

MoKob changed the title ~~Via way restrictions, resolves #2681~~ Via way restrictions Jul 24, 2017

MoKob mentioned this pull request Jul 24, 2017

Via way traffic signals #4327

Merged

5 tasks

MoKob force-pushed the via-way-restrictions branch from 6109840 to 6f81748 Compare July 24, 2017 14:49

daniel-j-h reviewed Jul 25, 2017

View reviewed changes

TheMarex requested changes Jul 25, 2017

View reviewed changes

willwhite mentioned this pull request Jul 26, 2017

Support conditional via way turn restrictions #4342

Closed

MoKob added Review - In feedback and removed Review labels Jul 27, 2017

MoKob force-pushed the via-way-restrictions branch 2 times, most recently from fdaee14 to 20ff5c5 Compare July 27, 2017 14:40

MoKob force-pushed the via-way-restrictions branch from 20ff5c5 to d024c8e Compare July 27, 2017 14:51

Moritz Kobitzsch added 6 commits July 28, 2017 14:38

adding tests to highlight via-way-restriciton development

4b67cc4

pre-filter turn restrictions for validity

fb4e9e5

Add data structure to allow identification of via-way turns during cr…

cce1d3b

…eation of edge-based-graph initial version of handling via-way turn restrictions (this is dirty) - requires update of data structures - requires clean-up - requires optimisation

add testcases to highlight limitation of via-way handling

a3c8a4e

restructure for review remarks

49e77ac

MoKob force-pushed the via-way-restrictions branch from d024c8e to 49e77ac Compare July 28, 2017 12:39

TheMarex approved these changes Jul 28, 2017

View reviewed changes

MoKob merged commit 8135f08 into master Jul 31, 2017

MoKob deleted the via-way-restrictions branch July 31, 2017 07:36

mjjbell mentioned this pull request Dec 9, 2020

Add support for multiple via-way restrictions #5907

Merged

6 tasks

mjjbell mentioned this pull request Jul 17, 2021

Generation of inefficient MLD partitions #6084

Closed

Via way restrictions #4255

Via way restrictions #4255

Conversation

MoKob commented Jul 7, 2017 • edited Loading

Issue

Tasklist

Performance:

Master

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

MoKob commented Jul 13, 2017

MoKob commented Jul 18, 2017

daniel-j-h 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

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 left a comment • 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

MoKob Jul 26, 2017 • 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

MoKob commented Jul 28, 2017

MoKob commented Jul 7, 2017 •

edited

Loading

TheMarex left a comment •

edited

Loading

MoKob Jul 26, 2017 •

edited

Loading