Refactor update routines #3809

TheMarex · 2017-03-11T02:02:21Z

Issue

This continue #3785 to make the update code easier to read and simplify logic.

On my test dataset the update speed was slightly faster then before, but the CSV parsing dominates almost all processing time.

Tasklist

review
Update wiki of changed files
adjust for comments

Relations

Implements #3737

daniel-j-h · 2017-03-16T09:41:09Z

CHANGELOG.md

@@ -4,6 +4,8 @@
      - Shared memory notification via conditional variables on Linux or semaphore queue on OS X and Windows with a limit of 128 OSRM Engine instances
    - Files
      - .osrm.datasource_index file was removed. Data is now part of .osrm.geometries.
+      - .osrm.edge_lookup was removed. The option `--generate-edge-lookup` does nothing now.
+      - `osrm-contract` does not depend on the `.osrm.fileIndex` file anymore


Can you update the wiki for these changes

daniel-j-h · 2017-03-16T09:42:08Z

features/support/shared_steps.js

                                        return cb(new Error('Annotation not found in response', a_type));
-                                    got[k] = annotation[a_type];
+                                    got[k] = annotation && annotation[a_type] || '';


Hm unrelated changes?

Noticed this while debugging a failing test. Cucumber crashes instead of just failing the test case.

daniel-j-h · 2017-03-16T09:42:43Z

include/contractor/graph_contractor_adaptors.hpp

@@ -20,6 +20,9 @@ std::vector<ContractorEdge> adaptToContractorInput(InputEdgeContainer input_edge

    for (const auto &input_edge : input_edge_list)
    {
+        if (input_edge.data.weight == INVALID_EDGE_WEIGHT)
+            continue;


Wait this is a bug in master? Should this be backported to the 5.6 branch?

No previously we filtered these edges out the edge loading routine. We were loading one struct at a time, so we could just skip them.

daniel-j-h · 2017-03-16T09:44:03Z

include/extractor/segment_data_container.hpp

    {
-        return fwd_durations[index[id] + 1 + offset];
+        const auto begin = nodes.begin() + index.at(id);
+        const auto end = nodes.begin() + index.at(id + 1);


i[id] vs i.at(id) - seems like you changed all subscript operators to at?

Good catch, don't remember doing this change. oO

daniel-j-h · 2017-03-16T09:44:33Z

include/extractor/segment_data_container.hpp

+    auto GetReverseGeometry(const DirectionalGeometryID id)
+    {
+        return boost::adaptors::reverse(GetForwardGeometry(id));
+    }


daniel-j-h · 2017-03-16T10:32:54Z

src/updater/updater.cpp

+                new_weight += weight;
+            }
+            const auto durations = segment_data.GetForwardDurations(geometry_id.id);
+            new_duration = std::accumulate(durations.begin(), durations.end(), 0);


daniel-j-h · 2017-03-16T10:34:02Z

src/updater/updater.cpp

+                if (weight == INVALID_EDGE_WEIGHT)
+                {
+                    new_weight = INVALID_EDGE_WEIGHT;
+                    break;


daniel-j-h · 2017-03-16T10:34:16Z

src/updater/updater.cpp

+                new_weight += weight;
+            }
+            const auto durations = segment_data.GetReverseDurations(geometry_id.id);
+            new_duration = std::accumulate(durations.begin(), durations.end(), 0);


daniel-j-h · 2017-03-16T10:35:42Z

src/updater/updater.cpp

+    if (updated_segments.size() > 0)
+    {
+        tbb::parallel_for(tbb::blocked_range<std::size_t>(0, edge_based_edge_list.size()),
+                          [&](const tbb::blocked_range<std::size_t> &range) {


daniel-j-h · 2017-03-16T10:35:51Z

src/updater/updater.cpp

+
+    std::vector<WeightAndDuration> accumulated_segment_data(updated_segments.size());
+    tbb::parallel_for(tbb::blocked_range<std::size_t>(0, updated_segments.size()),
+                      [&](const tbb::blocked_range<std::size_t> &range) {


TheMarex · 2017-03-16T21:49:23Z

@daniel-j-h should have addressed all your comments, let me know if this works for you.

oxidase

One fix is required, also it fixes already #3831, but osrm-contract with sanitizer still can hang

EDIT: looks like sanitizer hangs at exit

(gdb) bt
#0  0x000000000068b06e in __sanitizer::BlockingMutex::Lock() ()
#1  0x00000000006763f3 in __asan::AppendToErrorMessageBuffer(char const*) ()
#2  0x000000000068d68a in __sanitizer::SharedPrintfCode(bool, char const*, __va_list_tag*) ()
#3  0x000000000068d804 in __sanitizer::Report(char const*, ...) ()
#4  0x000000000069ec20 in __lsan::CheckForLeaksCallback(__sanitizer::SuspendedThreadsList const&, void*) ()
#5  0x000000000069b57d in __sanitizer::TracerThread(void*) ()
#6  0x000000000068bb87 in __sanitizer::internal_clone(int (*)(void*), void*, int, void*, int*, void*, int*) ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

oxidase · 2017-03-17T10:17:44Z

src/updater/updater.cpp

+            auto fwd_durations_range = segment_data.GetForwardDurations(geometry_id);
+            auto fwd_datasources_range = segment_data.GetForwardDatasources(geometry_id);
+            bool fwd_was_updated = false;
+            for (const auto segment_offset : util::irange(0, fwd_weights_range.size()))


SFINAE fails for (int, size_t) -> util::irange<std::size_t>(0, fwd_weights_range.size()))

oxidase · 2017-03-17T10:18:15Z

src/updater/updater.cpp

+                boost::adaptors::reverse(segment_data.GetReverseDatasources(geometry_id));
+            bool rev_was_updated = false;
+
+            for (const auto segment_offset : util::irange(0, rev_weights_range.size()))


TheMarex added the Work In Progress label Mar 11, 2017

TheMarex force-pushed the refactor/split-updater branch 6 times, most recently from 943f3fb to f8a1cf6 Compare March 14, 2017 21:26

TheMarex added Review and removed Work In Progress labels Mar 15, 2017

TheMarex requested review from daniel-j-h and danpat March 15, 2017 22:29

TheMarex force-pushed the refactor/split-updater branch 2 times, most recently from 395c51c to bc8a4c5 Compare March 15, 2017 23:53

daniel-j-h requested changes Mar 16, 2017

View reviewed changes

TheMarex added Review - In feedback and removed Review labels Mar 16, 2017

daniel-j-h approved these changes Mar 17, 2017

View reviewed changes

oxidase requested changes Mar 17, 2017

View reviewed changes

oxidase mentioned this pull request Mar 17, 2017

MLD shortest direct path search #3788

Merged

5 tasks

TheMarex added 10 commits March 17, 2017 10:39

Split out segment data update function

2122ef1

Split segment data update and edge update into sub-functions

eae9689

Make segment data update work without rtree

374213b

Update consistency checker

79ddd71

Fix crash in support code when there are no annotations

3fcb356

Don't copy query nodes

fff1a3b

Refactor update factor warning

7554683

Return sorted list of updated segments and segment_data

488f4c5

Only recompute weights for segments that needs to be updated

444d2bf

Remove .osrm.edge_segment_update not needed anymore

3a9d750

TheMarex added 8 commits March 17, 2017 10:39

First step of moving turn penalty update to own function

6909dcb

Remove duration fallback for turn penalties to simplify logic

13693c3

Consolidate data loading

a63068e

Fix invalid warning

65b505c

Precompute accumulated segment data

f301eac

Load edge based graph to memory in one block and then update

83af82d

Ignore --generate-edge-lookup and not list it anymore

8e57c9a

Address @daniel-j-h PR commtents

0d33e3e

TheMarex force-pushed the refactor/split-updater branch from e7b937b to 0d33e3e Compare March 17, 2017 10:41

oxidase approved these changes Mar 17, 2017

View reviewed changes

TheMarex merged commit bf6698f into master Mar 17, 2017

TheMarex deleted the refactor/split-updater branch March 17, 2017 11:20

TheMarex added Ready To Merge and removed Review - In feedback labels Mar 17, 2017

oxidase mentioned this pull request Mar 17, 2017

clang 4 sanitizer is not happy with TBB usage in updater #3831

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor update routines #3809

Refactor update routines #3809

TheMarex commented Mar 11, 2017 •

edited

Loading

daniel-j-h Mar 16, 2017

daniel-j-h Mar 16, 2017

TheMarex Mar 16, 2017

daniel-j-h Mar 16, 2017

TheMarex Mar 16, 2017 •

edited

Loading

daniel-j-h Mar 16, 2017

TheMarex Mar 16, 2017

daniel-j-h Mar 16, 2017

daniel-j-h Mar 16, 2017

daniel-j-h Mar 16, 2017

daniel-j-h Mar 16, 2017

daniel-j-h Mar 16, 2017

daniel-j-h Mar 16, 2017

TheMarex commented Mar 16, 2017

oxidase left a comment •

edited

Loading

oxidase Mar 17, 2017

oxidase Mar 17, 2017

Refactor update routines #3809

Refactor update routines #3809

Conversation

TheMarex commented Mar 11, 2017 • edited Loading

Issue

Tasklist

Relations

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 Mar 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMarex commented Mar 16, 2017

oxidase 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

TheMarex commented Mar 11, 2017 •

edited

Loading

TheMarex Mar 16, 2017 •

edited

Loading

oxidase left a comment •

edited

Loading