Renumber graph nodes after partitioning them #4089

TheMarex · 2017-05-24T22:30:12Z

Issue

This implements the second part of #3779

Tasklist

Requirements / Relations

This PR is rebased on #4056.

TheMarex · 2017-05-25T00:30:28Z

Some tests are failing, need to investigate those.

oxidase

LGTM, but some small open questions

oxidase · 2017-05-26T11:37:48Z

include/partition/edge_based_graph_reader.hpp

+            for (auto edge : edge_based_graph.GetAdjacentEdgeRange(node))
+            {
+                const auto &data = edge_based_graph.GetEdgeData(edge);
+                max_turn_id = std::max(max_turn_id, data.turn_id);


Here can be a data race. It can fixed by

max_turn_id is atomic (bad)

use thread local values

use tbb::parallel_reduce

Urg good catch, that was a stupid mistake on my side.

oxidase · 2017-05-26T11:44:01Z

src/partition/renumber.cpp

+                auto target = graph.GetTarget(edge);
+                if (partition[node] != partition[target])
+                {
+                    border_level[node] = level;


but why not border_level[node] = std::max(border_level[node], level);?
or level is always non-decreasing -> assertion border_level[node] <= level;

oxidase · 2017-05-26T11:49:02Z

src/partition/renumber.cpp

+
+    for (const auto &partition : partitions)
+    {
+        std::stable_sort(


please add in words what is this sort is doing and

oxidase · 2017-05-26T11:49:47Z

src/partition/renumber.cpp

+    }
+
+    auto border_level = getHighestBorderLevel(graph, partitions);
+    std::stable_sort(


.. what this sort adjusts after the first ordering.

oxidase · 2017-05-26T12:00:23Z

include/util/dynamic_graph.hpp

+                old_to_new_edge[edge] = new_edge_index++;
+            }
+            // and all adjacent empty edges
+            for (auto edge = EndEdges(node); edge < number_of_edges && isDummy(edge); edge++)


just don't understand why dummy edges are needed? If it is only to have correct results of GetAdjacentEdgeRange and EndEdges then it is better to make an iteration over node_array directly and remove empty edges.

I mainly wanted the renumber operation to not interfere with the structure of the dynamic graph. Removing the dummy edges would compactify the structure but would have non-obvious side-effects like for example InsertEdge would now take O(n) because it needs to resize+copy the edge array and relocate the source node edges.

We might want an additional function shrink_to_fit or something like that, but I don't think we have good use for it right now.

oxidase · 2017-05-26T12:16:07Z

@TheMarex tests fail for CH algorithms only in cases where MLD ordering is really used, probably some CH data also has to be reordered or data preprocessing must be split between CH and MLD steps

EDIT: swapping

osrm-backend/features/support/data.js

Lines 246 to 247 in 6777b5c

    
           queue.defer(this.contractData.bind(this), p); 
        
           queue.defer(this.partitionData.bind(this), p);

will "fix" tests, but in general the problem will remain, CH data depends on ordering by graph partitioning

TheMarex · 2017-05-26T13:44:29Z

tests fail for CH algorithms only in cases where MLD ordering is really used, probably some CH data also has to be reordered or data preprocessing must be split between CH and MLD steps

Ah good point forgot that running contract before partition is also an option. I'll add some code that detects and .hsgr file and reorders it as well if present, to guarantee consistency.

TheMarex · 2017-05-31T23:35:29Z

Did some measurements today:

The memory access pattern for MLD improves significantly: It is concentrated around the low ids which are the border nodes. For CH we see some improved concentration as well. For MLD this translates into speedups, for CH there is no impact as far as I can tell.

Memory acces

Some plots that show the node id space on the x-axis and the time-slice on the y-axis. Basically these are snapshots of the accessed graph nodes very 10000 operations.

CH

Before:

After:

MLD

Before:

After:

Runtime

Left before renumbering, right after renumbering. These are for random routes on Bayern, measuring the whole HTTP request time with osrm-runner.

CH

MLD

TheMarex · 2017-06-01T00:29:59Z

The only failing test is https://github.com/Project-OSRM/osrm-backend/blob/master/features/testbot/zero-speed-updates.feature#L28 one of the zero-speed segment tests. Curiously it now returns a match. Might be related to the fact that the order in which osrm-contract and osrm-partition are run was changed.

oxidase · 2017-06-01T07:06:05Z

@TheMarex ~22% speedup just from renumbering looks really awesome! I have just few remarks:

histogram values can be visualized in log-scale otherwise it is almost pure blue pattern
it is better to check speedup and access patterns on large-range >1000km qureies
zero-speed-updates.feature tests really depends on snapping and renumbering changes nodes order, so either a oneway flag or bearing must be added to the test. Or simply add @todo and "FIXME: Refactor insertion of source and target phantom nodes #4086"

TheMarex · 2017-06-01T11:36:44Z

@oxidase investigated the failing test case:

We start on the reverse node (valid source and target since we only change the weight of 2,3 not 3,2)
We do a u-turn on the forward node (both forward and reverse search)
Search terminates since both forward + reverse search have met

This yields a path that includes an invalid weight.

I don't get how this test case was ever passing. There are only two nodes in the graph, renumbering should not have any effect on the graph structure. Also the rtree snapping is unique in this case since we don't place the coordinates on the nodes but in the middle of the segments.

oxidase · 2017-06-01T11:57:22Z

@TheMarex idk atm why the response is Ok instead of NoMatch, basically these tests must be revisited in #4086

In the general a problem may occur if source node is a meeting node than a negative weight value of the source node is used to compute new_weight:

If new_weight < 0 the upper bound is ignored
new_weight >= 0 than incorrect path can be selected ifreverse_heap.GetKey(node) >= weight

oxidase · 2017-06-02T10:21:56Z

@TheMarex is the fix of the failing test here

osrm-backend/include/partition/edge_based_graph_reader.hpp

Lines 174 to 175 in 5c7288a

    
           edges[data.turn_id].data.forward = true; 
        
           edges[data.turn_id].data.backward = false;

? It seems the fix is needed also for #4086

TheMarex · 2017-06-02T12:13:44Z

@oxidase #4086 should not be impacted because this bug only manifested when you wrote out the .ebg in osrm-partition again. Basically it was a mismatch between the format that osrm-extract produces and what osrm-partition wrote out after renumbering.

The new numbering uses the partition information to sort border nodes first to compactify storages that need access indexed by border node ID. We also get an optimized cache performance for free sincr we can also recursively sort the nodes by cell ID. This implements issue #3779.

TheMarex added the Work In Progress label May 24, 2017

TheMarex added this to the 5.8.0 milestone May 24, 2017

TheMarex force-pushed the feature/renumber-graph branch from 9628aee to 5c61a92 Compare May 25, 2017 00:29

TheMarex added Review and removed Work In Progress labels May 25, 2017

oxidase requested changes May 26, 2017

View reviewed changes

TheMarex added Review - In feedback and removed Review labels May 26, 2017

oxidase approved these changes May 26, 2017

View reviewed changes

TheMarex force-pushed the feature/renumber-graph branch 2 times, most recently from 4745e0c to 602dd39 Compare May 31, 2017 22:20

TheMarex force-pushed the feature/renumber-graph branch from 5c7288a to 5076ccd Compare June 2, 2017 15:28

TheMarex added 3 commits June 2, 2017 16:09

Small fixes for osrm-runner

f184eb7

Don't use manual extract commands in weight tests

6986153

TheMarex force-pushed the feature/renumber-graph branch from 5076ccd to 6986153 Compare June 2, 2017 16:09

TheMarex merged commit 6c7f0ed into master Jun 2, 2017

TheMarex deleted the feature/renumber-graph branch June 2, 2017 18:12

TheMarex mentioned this pull request Jun 8, 2017

Graph data structure for MLD #3779

Closed

oxidase mentioned this pull request Jun 29, 2017

Multiple runs of osrm-partition lead to OOM kills #4214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renumber graph nodes after partitioning them #4089

Renumber graph nodes after partitioning them #4089

TheMarex commented May 24, 2017 •

edited

Loading

TheMarex commented May 25, 2017

oxidase left a comment

oxidase May 26, 2017

TheMarex May 26, 2017

oxidase May 26, 2017

oxidase May 26, 2017

oxidase May 26, 2017

oxidase May 26, 2017

TheMarex May 26, 2017

oxidase commented May 26, 2017 •

edited

Loading

TheMarex commented May 26, 2017

TheMarex commented May 31, 2017

TheMarex commented Jun 1, 2017

oxidase commented Jun 1, 2017

TheMarex commented Jun 1, 2017

oxidase commented Jun 1, 2017

oxidase commented Jun 2, 2017

TheMarex commented Jun 2, 2017

Renumber graph nodes after partitioning them #4089

Renumber graph nodes after partitioning them #4089

Conversation

TheMarex commented May 24, 2017 • edited Loading

Issue

Tasklist

Requirements / Relations

TheMarex commented May 25, 2017

oxidase left a comment

Choose a reason for hiding this comment

oxidase May 26, 2017

Choose a reason for hiding this comment

TheMarex May 26, 2017

Choose a reason for hiding this comment

oxidase May 26, 2017

Choose a reason for hiding this comment

oxidase May 26, 2017

Choose a reason for hiding this comment

oxidase May 26, 2017

Choose a reason for hiding this comment

oxidase May 26, 2017

Choose a reason for hiding this comment

TheMarex May 26, 2017

Choose a reason for hiding this comment

oxidase commented May 26, 2017 • edited Loading

TheMarex commented May 26, 2017

TheMarex commented May 31, 2017

Memory acces

CH

MLD

Runtime

CH

MLD

TheMarex commented Jun 1, 2017

oxidase commented Jun 1, 2017

TheMarex commented Jun 1, 2017

oxidase commented Jun 1, 2017

oxidase commented Jun 2, 2017

TheMarex commented Jun 2, 2017

TheMarex commented May 24, 2017 •

edited

Loading

oxidase commented May 26, 2017 •

edited

Loading