Improve speed of Map Matching #5060

TheMarex · 2018-04-27T05:41:53Z

Issue

This PR optimizes the map matching plugin for speed. It uses two things to do that:

The distance annotation work and improved distance calculation speed
Caching the forward search space for every state transition.

For my test trace this yielded a speedup of 2.

Measurements

branch	avg. time	speedup
`master`	61.18	1.0
`faster_mapmatching`	35.44	1.81

Tasklist

run benchmark on real traces with bigger dataset
review
adjust for comments
cherry pick to release branch

daniel-j-h

Some Travis builds are timing out; and looks like a test is failing. Left some comments inline.

daniel-j-h · 2018-07-20T16:22:10Z

features/testbot/matching.feature

Why do these tests change when the PR says it's only optimizing the map-matching performance?

It impacts numeric stability a little bit, which can leas to different matchings being chosen.

daniel-j-h · 2018-07-20T16:23:12Z

include/engine/routing_algorithms/routing_base.hpp

+namespace detail
+{
+template <typename Algorithm>
+void insertSourceInHeap(typename SearchEngineData<Algorithm>::ManyToManyQueryHeap &heap, const PhantomNode &phantom_node)


Hm, why only template the algorithm and not simply the heap? Does it make sense to restricti it to a specific heap impl.?

We need this in order to provide overloads for the normal QueryHeap and ManyToMany heap.

daniel-j-h · 2018-07-20T16:25:09Z

include/engine/routing_algorithms/routing_base.hpp

+    }
+}
+}
+


Below: looks like we are doing the same for all algorithms? Do we need to be specific about the algorithm then or can we simply have generic functions passing the parameters on to the downstream functions?

The problem is that we can't infer the algorithm from the SearchEngineData since we only pass in sub-type typename SearchEngineData<Algorithm>::QueryHeap. I would love to do this with automatic code generation but I did not find a way to do it?

Hm can you simply put a member type into the sub-class as in vector<T>::value_type ?

daniel-j-h · 2018-07-20T16:25:42Z

include/engine/routing_algorithms/routing_base.hpp

+}
+
+template <typename Heap>
+void insertNodesInHeaps(Heap &forward_heap, Heap &reverse_heap, const PhantomNodes &nodes)


Here we template the heap in contrast to above where we keep the heap impl. fixed and only template the algorithm - why?

This a generic wrapper that dispatches to the overloaded functions for insert{Source/Target}InHeap.

daniel-j-h · 2018-07-20T16:26:38Z

include/engine/routing_algorithms/routing_base.hpp

+void adjustPathDistanceToPhantomNodes(const std::vector<NodeID> &path,
+                                      const PhantomNode &source_phantom,
+                                      const PhantomNode &target_phantom,
+                                      EdgeDistance &distance);


out-ref? can you return by value?

daniel-j-h · 2018-07-20T16:32:03Z

src/engine/routing_algorithms/routing_base.cpp

+        }
+    }
+
+    // guard against underflow errors caused by rounding


Should there be an assert here checking for an epsilon making sure it's only due to rounding and not due to bugs in our code? Otherwise a negative erroneous distance will be silently ignored here.

daniel-j-h · 2018-07-20T16:32:38Z

src/engine/routing_algorithms/routing_base_ch.cpp

@@ -110,10 +110,14 @@ void search(SearchEngineData<Algorithm> & /*engine_working_data*/,
    weight = weight_upper_bound;

    // get offset to account for offsets on phantom nodes on compressed edges
-    const auto min_edge_offset = std::min(0, forward_heap.MinKey());
-    BOOST_ASSERT(min_edge_offset <= 0);
+    auto min_edge_offset = 0;


This will be int - do we want to be explicit about the type here?

daniel-j-h · 2018-07-20T16:33:09Z

src/engine/routing_algorithms/routing_base_ch.cpp

+        unpackPath(facade,
+                   packed_path.begin(),
+                   packed_path.end(),
+                   [&](std::pair<NodeID, NodeID> &edge, const auto &) {


const ref for the pair? Maybe const auto& even

daniel-j-h · 2018-07-20T16:33:25Z

src/engine/routing_algorithms/routing_base_ch.cpp

+                       unpacked_nodes.push_back(edge.second);
+                   });
+
+        for (auto node_iter = unpacked_nodes.begin(); node_iter != std::prev(unpacked_nodes.end());


Same as above: last and accumulate

daniel-j-h · 2018-07-20T16:34:04Z

src/util/coordinate_calculation.cpp

+
+// earth radius varies between 6,356.750-6,378.135 km (3,949.901-3,963.189mi)
+// The IUGG value for the equatorial radius is 6378.137 km (3963.19 miles)
+const constexpr long double EARTH_RADIUS = 6372797.560856;


I don't think this has to be long double? Why do we need it?

You are right up to 15 digits should be fine.

TheMarex · 2018-07-22T15:49:01Z

@daniel-j-h Addressed your PR comments and added a measurements for the speedups on Berlin.

daniel-j-h

Looks like cucumber is currently still failing and you need to re-run clang-format. Otherwise 👍 🚢

TheMarex added the Review label Apr 27, 2018

TheMarex force-pushed the faster_mapmatching branch from 0eab1f2 to f8c478f Compare June 27, 2018 20:40

TheMarex added 2 commits July 20, 2018 12:40

Use FCC algorithm for map matching distance calculation

d14a0ad

Use distance functions from many to many

5f0cc1d

TheMarex force-pushed the faster_mapmatching branch from f8c478f to 641906d Compare July 20, 2018 12:40

daniel-j-h reviewed Jul 20, 2018

View reviewed changes

TheMarex added Review - In feedback and removed Review labels Jul 22, 2018

TheMarex added 3 commits July 22, 2018 15:29

Preserve heap state in map matching

172877c

Round network distance to deci-meter to retain previous behavior

f69d78b

Adjust text cases for flightly different matching due to rounding

edd334d

TheMarex force-pushed the faster_mapmatching branch from 2f84f37 to edd334d Compare July 22, 2018 15:30

daniel-j-h reviewed Jul 23, 2018

View reviewed changes

TheMarex added 2 commits July 24, 2018 12:39

Fix bug in computation of distance offset for phantom node

6f3665f

Fix formating

66f0d8f

TheMarex mentioned this pull request Jul 24, 2018

The 'table' API can return negative distances. #5106

Closed

TheMarex added this to the 5.18.1 milestone Jul 24, 2018

Update changelog

107d0ea

TheMarex force-pushed the faster_mapmatching branch from e996e5d to 107d0ea Compare July 24, 2018 14:23

TheMarex merged commit 9b779c7 into master Aug 2, 2018

TheMarex deleted the faster_mapmatching branch August 2, 2018 10:19

rgugliel mentioned this pull request Aug 15, 2018

Difference in distance when using route and table #5151

Closed

danpat added a commit that referenced this pull request Aug 30, 2018

Rebased on 5.18 to avoid bugs introduced in #5060

4acce59

This was referenced Sep 5, 2018

OSRM crashes on mapmatching request #5195

Open

Revert "Improve speed of Map Matching" #5196

Merged

mjjbell mentioned this pull request Jun 8, 2021

Issue with Trip service #5647

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve speed of Map Matching #5060

Improve speed of Map Matching #5060

TheMarex commented Apr 27, 2018 •

edited

Loading

daniel-j-h left a comment

daniel-j-h Jul 20, 2018

TheMarex Jul 22, 2018

daniel-j-h Jul 20, 2018

TheMarex Jul 22, 2018

daniel-j-h Jul 20, 2018

TheMarex Jul 22, 2018

daniel-j-h Jul 23, 2018

daniel-j-h Jul 20, 2018

TheMarex Jul 22, 2018

daniel-j-h Jul 20, 2018

daniel-j-h Jul 20, 2018

daniel-j-h Jul 20, 2018

daniel-j-h Jul 20, 2018

daniel-j-h Jul 20, 2018

daniel-j-h Jul 20, 2018

TheMarex Jul 22, 2018

TheMarex commented Jul 22, 2018

daniel-j-h left a comment

Improve speed of Map Matching #5060

Improve speed of Map Matching #5060

Conversation

TheMarex commented Apr 27, 2018 • edited Loading

Issue

Measurements

Tasklist

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 commented Jul 22, 2018

daniel-j-h left a comment

Choose a reason for hiding this comment

TheMarex commented Apr 27, 2018 •

edited

Loading