Skip to content
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

Optimize RestrictionParser performance #6344

Merged
merged 13 commits into from
Aug 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- NodeJS:
- FIXED: Support `skip_waypoints` in Node bindings [#6060](https://github.com/Project-OSRM/osrm-backend/pull/6060)
- Misc:
- CHANGED: Optimize RestrictionParser performance. [#6344](https://github.com/Project-OSRM/osrm-backend/pull/6344)
- ADDED: Support floats for speed value in traffic updates CSV. [#6327](https://github.com/Project-OSRM/osrm-backend/pull/6327)
- CHANGED: Use Lua 5.4 in Docker image. [#6346](https://github.com/Project-OSRM/osrm-backend/pull/6346)
- CHANGED: Remove redundant nullptr check. [#6326](https://github.com/Project-OSRM/osrm-backend/pull/6326)
Expand Down
7 changes: 5 additions & 2 deletions include/extractor/restriction_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

#include <boost/optional/optional.hpp>

#include <osmium/tags/filter.hpp>
#include <set>
#include <string>
#include <vector>

Expand Down Expand Up @@ -43,15 +45,16 @@ class RestrictionParser
public:
RestrictionParser(bool use_turn_restrictions,
bool parse_conditionals,
std::vector<std::string> &restrictions);
const std::vector<std::string> &restrictions);
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
} // namespace extractor
} // namespace osrm
Expand Down
152 changes: 77 additions & 75 deletions src/extractor/restriction_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@
#include "extractor/profile_properties.hpp"

#include "util/conditional_restrictions.hpp"
#include "util/integer_range.hpp"
#include "util/log.hpp"

#include <boost/algorithm/string/predicate.hpp>
#include <boost/optional/optional.hpp>
#include <boost/ref.hpp>

#include <osmium/osm.hpp>
#include <osmium/tags/regex_filter.hpp>

#include <algorithm>

Expand All @@ -20,9 +20,9 @@ namespace extractor

RestrictionParser::RestrictionParser(bool use_turn_restrictions_,
bool parse_conditionals_,
std::vector<std::string> &restrictions_)
const std::vector<std::string> &restrictions_)
: use_turn_restrictions(use_turn_restrictions_), parse_conditionals(parse_conditionals_),
restrictions(restrictions_)
restrictions(restrictions_.begin(), restrictions_.end()), filter(false)
{
if (use_turn_restrictions)
{
Expand All @@ -40,11 +40,28 @@ RestrictionParser::RestrictionParser(bool use_turn_restrictions_,
util::Log() << "Found no turn restriction tags";
}
}

filter.add(true, "restriction");
if (parse_conditionals)
{
filter.add(true, "restriction:conditional");
for (const auto &namespaced : restrictions_)
{
filter.add(true, "restriction:" + namespaced + ":conditional");
}
}

// Not only use restriction= but also e.g. restriction:motorcar=
// Include restriction:{mode}:conditional if flagged
for (const auto &namespaced : restrictions_)
{
filter.add(true, "restriction:" + namespaced);
}
}

/**
* Tries to parse a relation as a turn restriction. This can fail for a number of
* reasons. The return type is a boost::optional<T>.
* reasons. The return type is a std::vector<T>.
*
* Some restrictions can also be ignored: See the ```get_restrictions``` function
* in the corresponding profile. We use it for both namespacing restrictions, as in
Expand All @@ -59,31 +76,13 @@ RestrictionParser::TryParse(const osmium::Relation &relation) const
return {};
}

osmium::tags::KeyFilter filter(false);
filter.add(true, "restriction");
if (parse_conditionals)
{
filter.add(true, "restriction:conditional");
for (const auto &namespaced : restrictions)
{
filter.add(true, "restriction:" + namespaced + ":conditional");
}
}

// Not only use restriction= but also e.g. restriction:motorcar=
// Include restriction:{mode}:conditional if flagged
for (const auto &namespaced : restrictions)
{
filter.add(true, "restriction:" + namespaced);
}

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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (fi_begin == fi_end)
{
return {};
}
Expand All @@ -99,18 +98,20 @@ RestrictionParser::TryParse(const osmium::Relation &relation) const
bool is_multi_from = false;
bool is_multi_to = false;

std::vector<util::OpeningHours> condition;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const std::string value(fi_begin->value());
auto value = fi_begin->value();

// documented OSM restriction tags start either with only_* or no_*;
// check and return on these values, and ignore no_*_on_red or unrecognized values
if (value.find("only_") == 0)
if (boost::algorithm::starts_with(value, "only_"))
{
is_only_restriction = true;
}
else if (value.find("no_") == 0 && !boost::algorithm::ends_with(value, "_on_red"))
else if (boost::algorithm::starts_with(value, "no_") &&
!boost::algorithm::ends_with(value, "_on_red"))
{
is_only_restriction = false;
if (boost::algorithm::starts_with(value, "no_exit"))
Expand All @@ -126,6 +127,25 @@ RestrictionParser::TryParse(const osmium::Relation &relation) const
{
return {};
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Aug 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@mjjbell mjjbell Aug 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if (parse_conditionals)
{
// Parse condition and add independent value/condition pairs
const auto &parsed = osrm::util::ParseConditionalRestrictions(value);

if (parsed.empty())
continue;

for (const auto &p : parsed)
{
std::vector<util::OpeningHours> hours = util::ParseOpeningHours(p.condition);
// found unrecognized condition, continue
if (hours.empty())
return {};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


condition = std::move(hours);
}
}
}

constexpr auto INVALID_OSM_ID = std::numeric_limits<std::uint64_t>::max();
Expand All @@ -138,7 +158,11 @@ RestrictionParser::TryParse(const osmium::Relation &relation) const
for (const auto &member : relation.members())
{
const char *role = member.role();
if (strcmp("from", role) != 0 && strcmp("to", role) != 0 && strcmp("via", role) != 0)
const bool is_from_role = strcmp("from", role) == 0;
const bool is_to_role = strcmp("to", role) == 0;
const bool is_via_role = strcmp("via", role) == 0;

if (!is_from_role && !is_to_role && !is_via_role)
{
continue;
}
Expand All @@ -149,28 +173,27 @@ RestrictionParser::TryParse(const osmium::Relation &relation) const
{

// Make sure nodes appear only in the role if a via node
if (0 == strcmp("from", role) || 0 == strcmp("to", role))
if (is_from_role || is_to_role)
{
continue;
}
BOOST_ASSERT(0 == strcmp("via", role));
BOOST_ASSERT(is_via_role);
via_node = static_cast<std::uint64_t>(member.ref());
is_node_restriction = true;
// set via node id
break;
}
case osmium::item_type::way:
BOOST_ASSERT(0 == strcmp("from", role) || 0 == strcmp("to", role) ||
0 == strcmp("via", role));
if (0 == strcmp("from", role))
BOOST_ASSERT(is_from_role || is_to_role || is_via_role);
if (is_from_role)
{
from_ways.push_back({static_cast<std::uint64_t>(member.ref())});
}
else if (0 == strcmp("to", role))
else if (is_to_role)
{
to_ways.push_back({static_cast<std::uint64_t>(member.ref())});
}
else if (0 == strcmp("via", role))
else if (is_via_role)
{
via_ways.push_back({static_cast<std::uint64_t>(member.ref())});
is_node_restriction = false;
Expand All @@ -185,35 +208,6 @@ RestrictionParser::TryParse(const osmium::Relation &relation) const
}
}

std::vector<util::OpeningHours> condition;
// parse conditional tags
if (parse_conditionals)
{
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());
for (; fi_begin != fi_end; ++fi_begin)
{
const std::string key(fi_begin->key());
const std::string value(fi_begin->value());

// Parse condition and add independent value/condition pairs
const auto &parsed = osrm::util::ParseConditionalRestrictions(value);

if (parsed.empty())
continue;

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

condition = std::move(hours);
}
}
}

std::vector<InputTurnRestriction> restriction_containers;
if (!from_ways.empty() && (via_node != INVALID_OSM_ID || !via_ways.empty()) && !to_ways.empty())
{
Expand Down Expand Up @@ -270,17 +264,25 @@ bool RestrictionParser::ShouldIgnoreRestriction(const std::string &except_tag_st
return false;
}

// Be warned, this is quadratic work here, but we assume that
// only a few exceptions are actually defined.
const std::regex delimiter_re("[;][ ]*");
std::sregex_token_iterator except_tags_begin(
except_tag_string.begin(), except_tag_string.end(), delimiter_re, -1);
std::sregex_token_iterator except_tags_end;

return std::any_of(except_tags_begin, except_tags_end, [&](const std::string &current_string) {
return std::end(restrictions) !=
std::find(std::begin(restrictions), std::end(restrictions), current_string);
});
// split `except_tag_string` by semicolon and check if any of items is in `restrictions`
std::string current_string;
for (auto index : util::irange<size_t>(0, except_tag_string.size()))
{
const auto ch = except_tag_string[index];
if (ch != ';')
{
current_string += ch;
}
else
{
if (restrictions.find(current_string) != restrictions.end())
{
return true;
}
current_string.clear();
}
}
return restrictions.find(current_string) != restrictions.end();
}
} // namespace extractor
} // namespace osrm