bug-fix: invalid contract node

[fenwuyaoji]

Issue

To fix issue #7054

Currently, in OSRM CH algo, the graph contract node and search routes update the weight of each edge. OSRM uses filters to exclude some specific roads with tags like toll, ferry, etc. In the function contractExcludableGraph, if filters are more than 1, OSRM will first contract a non-core graph and contract a corresponding graph of each filter. However, the weight update process uses all edges to search routes and doesn't consider that we can not use edges that should be excluded. This will lead to no route found issue when using the exlclude=toll, addressed in #7054.
In that case, node 3 has 2 alternative routes to node 7, while 3->1->0->6->7 is faster than 3->2->0. When OSRM wants to contract node 2, edge (3,2), (7,2) are deleted and no shortcut is inserted, because 3->1->0->6->7 is the actual shortcut (shorter than 3->2->0). However, updating the route weight doesn’t consider whether node 0 or 1 or others are accessible in this filtered graph. So I try to pass the array contractable into function relaxNode(), if a finding node is inaccessible, we shouldn’t insert this node into the searching heap or update the weight.
Notice: this change will leave more edges in graph and enlarge the memory usage, but I think it's worth it.

I'm not sure this is the best way to fix it. Waiting for your feedback, and appreciate it in advance.

Please read our documentation on release and version management.
If your PR is still work in progress please attach the relevant label.

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?

[DennisOSRM]

requires a formatting change for CI to proceed:

src/contractor/contractor_search.cpp src/contractor/graph_contractor.cpp
diff --git a/src/contractor/contractor_search.cpp b/src/contractor/contractor_search.cpp
index 8226e51..620cd2a 100644
--- a/src/contractor/contractor_search.cpp
+++ b/src/contractor/contractor_search.cpp
@@ -35,7 +35,8 @@ void relaxNode(ContractorHeap &heap,
         // New Node discovered -> Add to Heap + Node Info Storage
         if (!toHeapNode)
         {
-            if (!contractable[to]) {
+            if (!contractable[to])
+            {
                 continue;
             }
             heap.Insert(to, to_weight, ContractorHeapData{current_hop, false});
diff --git a/src/contractor/graph_contractor.cpp b/src/contractor/graph_contractor.cpp
index c449590..6a32182 100644
--- a/src/contractor/graph_contractor.cpp
+++ b/src/contractor/graph_contractor.cpp
@@ -246,12 +246,24 @@ void ContractNode(ContractorThreadData *data,
         if (RUNSIMULATION)
         {
             const int constexpr SIMULATION_SEARCH_SPACE_SIZE = 1000;
-            search(heap, graph, contractable, number_of_targets, SIMULATION_SEARCH_SPACE_SIZE, max_weight, node);
+            search(heap,
+                   graph,
+                   contractable,
+                   number_of_targets,
+                   SIMULATION_SEARCH_SPACE_SIZE,
+                   max_weight,
+                   node);
         }
         else
         {
             const int constexpr FULL_SEARCH_SPACE_SIZE = 2000;
-            search(heap, graph, contractable, number_of_targets, FULL_SEARCH_SPACE_SIZE, max_weight, node);
+            search(heap,
+                   graph,
+                   contractable,
+                   number_of_targets,
+                   FULL_SEARCH_SPACE_SIZE,
+                   max_weight,
+                   node);
         }
         for (auto out_edge : graph.GetAdjacentEdgeRange(node))
         {
@@ -490,7 +502,8 @@ bool UpdateNodeNeighbours(ContractorNodeData &node_data,
         if (node_data.contractable[u])
         {
             node_data.priorities[u] = EvaluateNodePriority(
-                SimulateNodeContraction(data, graph, u, node_data.weights, node_data.contractable), node_data.depths[u]);
+                SimulateNodeContraction(data, graph, u, node_data.weights, node_data.contractable),
+                node_data.depths[u]);
         }
     }
     return true;
@@ -621,19 +634,21 @@ std::vector<bool> contractGraph(ContractorGraph &graph,
     {
         util::UnbufferedLog log;
         log << "initializing node priorities...";
-        tbb::parallel_for(tbb::blocked_range<std::size_t>(0, remaining_nodes.size(), PQGrainSize),
-                          [&](const auto &range)
-                          {
-                              ContractorThreadData *data = thread_data_list.GetThreadData();
-                              for (auto x = range.begin(), end = range.end(); x != end; ++x)
-                              {
-                                  auto node = remaining_nodes[x].id;
-                                  BOOST_ASSERT(node_data.contractable[node]);
-                                  node_data.priorities[node] = EvaluateNodePriority(
-                                      SimulateNodeContraction(data, graph, node, node_data.weights, node_data.contractable),
-                                      node_data.depths[node]);
-                              }
-                          });
+        tbb::parallel_for(
+            tbb::blocked_range<std::size_t>(0, remaining_nodes.size(), PQGrainSize),
+            [&](const auto &range)
+            {
+                ContractorThreadData *data = thread_data_list.GetThreadData();
+                for (auto x = range.begin(), end = range.end(); x != end; ++x)
+                {
+                    auto node = remaining_nodes[x].id;
+                    BOOST_ASSERT(node_data.contractable[node]);
+                    node_data.priorities[node] = EvaluateNodePriority(
+                        SimulateNodeContraction(
+                            data, graph, node, node_data.weights, node_data.contractable),
+                        node_data.depths[node]);
+                }
+            });
         log << " ok.";
     }

[fenwuyaoji]

@DennisOSRM Thanks for your kindly reply, I just fixed the format issue, plz have a look again

[DennisOSRM]

Still failing. Please have a look at the failing test output.

[fenwuyaoji]

@DennisOSRM . I've run on ubuntu with docker and passed the pre-push hook. plz check again. sry to bother you again

[fenwuyaoji]

@DennisOSRM thank you. all checks passed, could you review this PR? is it ok to merge?

[DennisOSRM]

I plan to review over the weekend.

[fenwuyaoji]

@DennisOSRM Hi, may I know whether you have any concerns about this fix?

[DennisOSRM]

could you add a test that covers the changed behaviour?

[fenwuyaoji]

@DennisOSRM Hi, I've added one, plz check, thank you.

[fenwuyaoji]

Should I add a comment in the Changelog 6.0.0 RC1 part?

[fenwuyaoji]

@DennisOSRM HI, I saw the test failed on Ubuntu, but I tested it successfully on my Mac. I'm not sure what the difference is, maybe the tbb behaves differently on these two envs? So I set max_allowed_parallelism as 1 in this test. could you trigger the workflow again? And do you have any ideas of this?

[DennisOSRM]

/home/runner/work/osrm-backend/osrm-backend/unit_tests/contractor/graph_contractor.cpp(85): fatal error: in "graph_contractor/contract_exclude_graph": critical check (query_graph2.GetAdjacentEdgeRange(1)).size() == 1 has failed [2 != 1]

Could this be an ordering issue in the input data?

[fenwuyaoji]

Finally, I found the reason is that the XOR_FAST_HASH behaves differently on Mac and Ubuntu, leading to different contract order and make the results different on two env.

[fenwuyaoji]

@DennisOSRM Hi, does the first graph in my test case illustrate the problem? If so, I will delete the graph2 or just remove the failed check to fix it. Or we need to input a specific random seed to std::mt19937 when initialize the generator.

osrm-backend/include/util/xor_fast_hash.hpp

Line 45 in 646384a

std::mt19937 generator; // impl. defined but deterministic default seed

@daniel-j-h Do you have any idea of this?

[daniel-j-h]

Don't know why you tag me here I haven't worked on this code base in 7+ years but here goes.

If you want the same pseudo-random number sequence across platforms you need to use the same seed, correct.

That said looking at the xor-fast-hash there are more issues with it

osrm-backend/include/util/xor_fast_hash.hpp

Lines 47 to 51 in 646384a

	std::iota(begin(table1), end(table1), 0u);
	std::shuffle(begin(table1), end(table1), generator);
	std::iota(begin(table2), end(table2), 0u);
	std::shuffle(begin(table2), end(table2), generator);

it is using tabulation hashing incorrectly: Instead of randomly initializing the tables it's initializing the tables with numbers from 0 to n and then shuffles them. This means you won't get the mathmatical properties from correct tabulation hashing.

In 2025 I can recommend just using a tried and proven hash function e.g. xxhash.

[fenwuyaoji]

Sorry to bother you, and thanks for your explanation. I just saw that you are the author, so I pinned you, haha.

[fenwuyaoji]

I just added an initial seed to guarantee the same behavior cross platforms. I think we need another PR to fix the problem Daniel mentioned.

[DennisOSRM]

I just added an initial seed to guarantee the same behavior cross platforms. I think we need another PR to fix the problem Daniel mentioned.

Yes, I can provide this initialization fix as a separate PR.