Optimize RestrictionParser performance #6344

SiarheiFedartsou · 2022-08-28T07:10:57Z

Issue

closes #4102

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

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-08-28T07:14:00Z

src/extractor/restriction_parser.cpp

    const osmium::TagList &tag_list = relation.tags();

    osmium::tags::KeyFilter::iterator fi_begin(filter, tag_list.begin(), tag_list.end());
    osmium::tags::KeyFilter::iterator fi_end(filter, tag_list.end(), tag_list.end());

    // if it's not a restriction, continue;
-    if (std::distance(fi_begin, fi_end) == 0)


It is O(N) for this iterator if I understand it correctly.

SiarheiFedartsou · 2022-08-28T07:14:56Z

src/extractor/restriction_parser.cpp

@@ -126,6 +126,25 @@ RestrictionParser::TryParse(const osmium::Relation &relation) const
        {
            return {};


Noticed that here we do return {} if found something unexpected in tag values, should we just ignore it instead? may be with some additional warning?

In most cases you'll get at most one restriction returned per relation, it's only no entry/exits that could have multiple.

I think the way to view it is, if the relation is invalid, how correct will the remaining parsed restrictions be? It's difficult to know, so it's cleaner to not to try and deal with them.

We could add a log, but I would only make it a debug level. There could be a lot of invalid OSM relations.

SiarheiFedartsou · 2022-08-28T07:15:25Z

src/extractor/restriction_parser.cpp

+                std::vector<util::OpeningHours> hours = util::ParseOpeningHours(p.condition);
+                // found unrecognized condition, continue
+                if (hours.empty())
+                    return {};


The same, if we found something unexpected we just return, should it be just ignoring instead?

SiarheiFedartsou · 2022-08-28T08:48:38Z

src/extractor/restriction_parser.cpp

    for (; fi_begin != fi_end; ++fi_begin)
    {
-        const std::string key(fi_begin->key());


Curious why didn’t we have warning about unused key variable here 🤔

mjjbell · 2022-08-28T19:41:56Z

include/extractor/restriction_parser.hpp

    std::vector<InputTurnRestriction> TryParse(const osmium::Relation &relation) const;

  private:
    bool ShouldIgnoreRestriction(const std::string &except_tag_string) const;

    bool use_turn_restrictions;
    bool parse_conditionals;
-    std::vector<std::string> restrictions;
+    std::set<std::string> restrictions;
+    osmium::tags::KeyFilter filter;


The Osmium docs say this is deprecated, and should be replaced with osmium::TagsFilter.

mjjbell · 2022-08-28T20:03:22Z

Can we run a benchmark on a reasonably sized OSM extract to evaluate the performance?

SiarheiFedartsou · 2022-08-29T16:26:26Z

Can we run a benchmark on a reasonably sized OSM extract to evaluate the performance?

Well, I ran it on 1.5GB poland-latest.osm.pbf(let me know if I should use something larger). I ran it for 3 times on both current master and this PR and used this value as a metric(because related code is executed only before this log):

On current master(on my M1 Pro + 32GB + 10 threads with ./osrm-extract -p ../profiles/car.lua poland-latest.osm.pbf):

(35.713 + 36.4149 + 37.1714) / 3 = 36.4331

With this PR:

(36.2307 + 35.4937 + 36.8094) / 3 = 36.1780

Results are definitely not impressive.

src/extractor/restriction_parser.cpp

mjjbell · 2022-08-29T18:49:58Z

Results are definitely not impressive.

It's to be expected given the number of restrictions is small relative to other elements being parsed. In any case, it's tidying up the code for future improvements 👍

Co-authored-by: Michael Bell <[email protected]>

SiarheiFedartsou · 2022-08-29T21:34:50Z

@mjjbell it seems this new TagsParser is incompatible with clang 6. I'd propose to go back to KeysFilter(as I understand bumping the compiler is not an option). WDYT?

mjjbell · 2022-08-29T21:39:12Z

I'd propose to go back to KeysFilter(as I understand bumping the compiler is not an option). WDYT?

👍

v5.27.0 - Changes from 5.26.0 - API: - ADDED: Add Flatbuffers support to NodeJS bindings. [Project-OSRM#6338](Project-OSRM#6338) - CHANGED: Add `data_version` field to responses of all services. [Project-OSRM#5387](Project-OSRM#5387) - FIXED: Use Boost.Beast to parse HTTP request. [Project-OSRM#6294](Project-OSRM#6294) - FIXED: Fix inefficient osrm-routed connection handling [Project-OSRM#6113](https://gihub.com/Project-OSRM/osrm-backend/pull/6113) - FIXED: Fix HTTP compression precedence [Project-OSRM#6113](Project-OSRM#6113) - NodeJS: - FIXED: Support `skip_waypoints` in Node bindings [Project-OSRM#6060](Project-OSRM#6060) - Misc: - ADDED: conanbuildinfo.json for easy reading of dependencies [Project-OSRM#6388](Project-OSRM#6388) - CHANGED: Improve performance of JSON rendering. Fix undefined behaviour in JSON numbers formatting. [Project-OSRM#6380](Project-OSRM#6380) - ADDED: Add timestamps for logs. [Project-OSRM#6375](Project-OSRM#6375) - CHANGED: Improve performance of map matching via getPathDistance optimization. [Project-OSRM#6378](Project-OSRM#6378) - CHANGED: Optimize RestrictionParser performance. [Project-OSRM#6344](Project-OSRM#6344) - ADDED: Support floats for speed value in traffic updates CSV. [Project-OSRM#6327](Project-OSRM#6327) - CHANGED: Use Lua 5.4 in Docker image. [Project-OSRM#6346](Project-OSRM#6346) - CHANGED: Remove redundant nullptr check. [Project-OSRM#6326](Project-OSRM#6326) - CHANGED: missing files list is included in exception message. [Project-OSRM#5360](Project-OSRM#5360) - CHANGED: Do not use deprecated Callback::Call overload in Node bindings. [Project-OSRM#6318](Project-OSRM#6318) - FIXED: Fix distance calculation consistency. [Project-OSRM#6315](Project-OSRM#6315) - FIXED: Fix performance issue after migration to sol2 3.3.0. [Project-OSRM#6304](Project-OSRM#6304) - CHANGED: Pass osm_node_ids by reference in osrm::updater::Updater class. [Project-OSRM#6298](Project-OSRM#6298) - FIXED: Fix bug with reading Set values from Lua scripts. [Project-OSRM#6285](Project-OSRM#6285) - FIXED: Bug in bicycle profile that caused exceptions if there is a highway=bicycle in the data. [Project-OSRM#6296](Project-OSRM#6296) - FIXED: Internal refactoring of identifier types used in data facade [Project-OSRM#6044](Project-OSRM#6044) - CHANGED: Update docs to reflect recent build and dependency changes [Project-OSRM#6383](Project-OSRM#6383) - Build: - REMOVED: Get rid of Mason. [Project-OSRM#6387](Project-OSRM#6387) - CHANGED: Use clang-format from CI base image. [Project-OSRM#6391](Project-OSRM#6391) - ADDED: Build Node bindings on Windows. [Project-OSRM#6334](Project-OSRM#6334) - ADDED: Configure cross-compilation for Apple Silicon. [Project-OSRM#6360](Project-OSRM#6360) - CHANGED: Use apt-get to install Clang on CI. [Project-OSRM#6345](Project-OSRM#6345) - CHANGED: Fix TBB in case of Conan + NodeJS build. [Project-OSRM#6333](Project-OSRM#6333) - CHANGED: Migrate to modern TBB version. [Project-OSRM#6300](Project-OSRM#6300) - CHANGED: Enable performance-move-const-arg clang-tidy check. [Project-OSRM#6319](Project-OSRM#6319) - CHANGED: Use the latest node on CI. [Project-OSRM#6317](Project-OSRM#6317) - CHANGED: Migrate Windows CI to GitHub Actions. [Project-OSRM#6312](Project-OSRM#6312) - ADDED: Add smoke test for Docker image. [Project-OSRM#6313](Project-OSRM#6313) - CHANGED: Update libosmium to version 2.18.0. [Project-OSRM#6303](Project-OSRM#6303) - CHANGED: Remove EXACT from find_package if using Conan. [Project-OSRM#6299](Project-OSRM#6299) - CHANGED: Configure Undefined Behaviour Sanitizer. [Project-OSRM#6290](Project-OSRM#6290) - CHANGED: Use Conan instead of Mason to install code dependencies. [Project-OSRM#6284](Project-OSRM#6284) - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [Project-OSRM#6279](Project-OSRM#6279) - CHANGED: Update macOS CI image to macos-11. [Project-OSRM#6286](Project-OSRM#6286) - CHANGED: Enable even more clang-tidy checks. [Project-OSRM#6273](Project-OSRM#6273) - CHANGED: Configure CMake to not build flatbuffers tests and samples. [Project-OSRM#6274](Project-OSRM#6274) - CHANGED: Enable more clang-tidy checks. [Project-OSRM#6270](Project-OSRM#6270) - CHANGED: Configure clang-tidy job on CI. [Project-OSRM#6261](Project-OSRM#6261) - CHANGED: Use Github Actions for building container images [Project-OSRM#6138](Project-OSRM#6138) - CHANGED: Upgrade Boost dependency to 1.70 [Project-OSRM#6113](Project-OSRM#6113) - CHANGED: Upgrade Ubuntu CI builds to 20.04 [Project-OSRM#6119](Project-OSRM#6119) - CHANGED: Make building osrm-routed optional [Project-OSRM#6144](Project-OSRM#6144) - FIXED: Run all unit tests in CI [Project-OSRM#5248](Project-OSRM#5248) - FIXED: Fix installation of Mason CMake and 32 bit CI build [Project-OSRM#6170](Project-OSRM#6170) - FIXED: Fixed Node docs generation check in CI. [Project-OSRM#6058](Project-OSRM#6058) - CHANGED: Docker build, enabled arm64 build layer [Project-OSRM#6172](Project-OSRM#6172) - CHANGED: Docker build, enabled apt-get update/install caching in separate layer for build phase [Project-OSRM#6175](Project-OSRM#6175) - FIXED: Bump CI complete meta job to ubuntu-20.04 [Project-OSRM#6323](Project-OSRM#6323) - CHANGED: Node packages are now scoped by @Project-OSRM [Project-OSRM#6386](Project-OSRM#6386) - Routing: - CHANGED: Lazily generate optional route path data [Project-OSRM#6045](Project-OSRM#6045) - FIXED: Completed support for no_entry and no_exit turn restrictions. [Project-OSRM#5988](Project-OSRM#5988) - ADDED: Add support for non-round-trips with a single fixed endpoint. [Project-OSRM#6050](Project-OSRM#6050) - FIXED: Improvements to maneuver override processing [Project-OSRM#6125](Project-OSRM#6125) - ADDED: Support snapping to multiple ways at an input location. [Project-OSRM#5953](Project-OSRM#5953) - FIXED: Fix snapping target locations to ways used in turn restrictions. [Project-OSRM#6339](Project-OSRM#6339) - ADDED: Support OSM traffic signal directions. [Project-OSRM#6153](Project-OSRM#6153) - FIXED: Ensure u-turn exists in intersection view. [Project-OSRM#6376](Project-OSRM#6376) - FIXED: Gracefully handle no-turn intersections in guidance processing. [Project-OSRM#6382](Project-OSRM#6382) - Profile: - CHANGED: Bicycle surface speeds [Project-OSRM#6212](Project-OSRM#6212) - Tools: - CHANGED: Do not generate intermediate .osrm file in osrm-extract. [Project-OSRM#6354](Project-OSRM#6354)

SiarheiFedartsou added 3 commits August 28, 2022 09:10

Optimize RestrictionParser performance

c14461c

Optimize RestrictionParser performance

01e2a2d

Optimize RestrictionParser performance

794ae59

SiarheiFedartsou force-pushed the sf-restriction-parser branch from 6a8439e to 794ae59 Compare August 28, 2022 07:13

SiarheiFedartsou commented Aug 28, 2022

View reviewed changes

Optimize RestrictionParser performance

cf223ef

SiarheiFedartsou marked this pull request as ready for review August 28, 2022 07:47

SiarheiFedartsou commented Aug 28, 2022

View reviewed changes

mjjbell reviewed Aug 28, 2022

View reviewed changes

Optimize RestrictionParser performance

1d86602

SiarheiFedartsou requested a review from mjjbell August 29, 2022 16:30

mjjbell approved these changes Aug 29, 2022

View reviewed changes

src/extractor/restriction_parser.cpp Outdated Show resolved Hide resolved

SiarheiFedartsou and others added 5 commits August 29, 2022 21:58

Update src/extractor/restriction_parser.cpp

ecbee99

Co-authored-by: Michael Bell <[email protected]>

Merge branch 'master' into sf-restriction-parser

c8f28fb

Update restriction_parser.cpp

ff704db

Update restriction_parser.cpp

70a507f

Optimize RestrictionParser performance

47f7ad7

SiarheiFedartsou added 3 commits August 30, 2022 16:30

Merge branch 'master' into sf-restriction-parser

94495b5

Optimize RestrictionParser performance

69a3843

Optimize RestrictionParser performance

7dfe466

SiarheiFedartsou merged commit 9a638f3 into master Aug 30, 2022

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Optimize RestrictionParser performance (Project-OSRM#6344)

d92718a

DennisOSRM deleted the sf-restriction-parser branch May 3, 2024 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize RestrictionParser performance #6344

Optimize RestrictionParser performance #6344

SiarheiFedartsou commented Aug 28, 2022 •

edited

Loading

SiarheiFedartsou Aug 28, 2022

SiarheiFedartsou Aug 28, 2022 •

edited

Loading

mjjbell Aug 28, 2022 •

edited

Loading

SiarheiFedartsou Aug 28, 2022

SiarheiFedartsou Aug 28, 2022

mjjbell Aug 28, 2022

mjjbell commented Aug 28, 2022

SiarheiFedartsou commented Aug 29, 2022 •

edited

Loading

mjjbell commented Aug 29, 2022

SiarheiFedartsou commented Aug 29, 2022

mjjbell commented Aug 29, 2022

		@@ -126,6 +126,25 @@ RestrictionParser::TryParse(const osmium::Relation &relation) const
		{
		return {};

Optimize RestrictionParser performance #6344

Optimize RestrictionParser performance #6344

Conversation

SiarheiFedartsou commented Aug 28, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

SiarheiFedartsou Aug 28, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Aug 28, 2022 • edited Loading

Choose a reason for hiding this comment

mjjbell Aug 28, 2022 • edited Loading

Choose a reason for hiding this comment

SiarheiFedartsou Aug 28, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Aug 28, 2022

Choose a reason for hiding this comment

mjjbell Aug 28, 2022

Choose a reason for hiding this comment

mjjbell commented Aug 28, 2022

SiarheiFedartsou commented Aug 29, 2022 • edited Loading

mjjbell commented Aug 29, 2022

SiarheiFedartsou commented Aug 29, 2022

mjjbell commented Aug 29, 2022

SiarheiFedartsou commented Aug 28, 2022 •

edited

Loading

SiarheiFedartsou Aug 28, 2022 •

edited

Loading

mjjbell Aug 28, 2022 •

edited

Loading

SiarheiFedartsou commented Aug 29, 2022 •

edited

Loading