-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Some issues in restrictions handling code #4102
Copy link
Copy link
Closed
Labels
Description
I had a look at the restrictions handling code and here are some mixed issues I found:
- https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L53 Wrong documentation, return type is a vector (https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L59) . That vector only ever contains at most one element!
- Building the KeyFilter https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L69-L85 is expensive, the filter should be built once and reused.
- Iterating over the tags with the filter is expensive because of the string comparisons. Doing this three times (https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L93 and https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L107 and https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L183) is suboptimal, as is creating strings just for a comparison (https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L110 and https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L186), or, even worse, not using them at all (https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L109 and https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L185).
- Checking the role multiple times (https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L133 and https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L142 for instance) is expensive. Better to only do this once.
- There must be a better way to split a string on semicolons than https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/restriction_parser.cpp#L231.
- Currently restrictions are read in the same go as all the other data (ie nodes and ways) which means they are read after nodes and ways, because OSM files are ordered that way. In other contexts (for instance multipolygon handling) it is much better to read first all relations and keep the information about them in memory and then, in a second pass, read the nodes and ways, matching each read object to the already read relations. There are only a few relations (compared to nodes and ways) and reading a planet file twice isn't as expensive as you might think, so this approach often works great. I don't know enough about the inner workings of OSRM, but that approach might help here, too. Especially when we want to support more complex restrictions (see Turn-Restrictions with via ways #2681). This could have a large influence in the lua-interface if that should support relations (see support relations in lua profiles #482).
Reactions are currently unavailable