fix(contractor): insert self-loops for all needed cases, not just one-ways#7442
Conversation
There was a problem hiding this comment.
Pull request overview
Reworks the contraction-hierarchy (CH) contractor to preserve shortest-path invariants for node-to-node distances and node-to-self loop distances by inserting self-loop shortcuts when appropriate.
Changes:
- Refactors the contractor implementation (parallel independent-node contraction, shortcut insertion, core handling) and updates APIs/call sites accordingly.
- Extends routing/assembly logic to consider self-loops in edge-based routing, and adds/adjusts unit + BDD tests.
- Adds heap/storage enhancements (LinearHashStorage option, IncreaseKey support) and introduces new debug/benchmark tooling/logging.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| unit_tests/util/query_heap.cpp | Expands heap storage backends tested (adds LinearHashStorage) and tweaks test sizes. |
| unit_tests/contractor/graph_contractor.cpp | Updates tests for new contractor APIs and adds assertions for self-loop behavior. |
| src/tools/routed.cpp | Makes std::exception handler unconditional (not Windows-only). |
| src/engine/routing_algorithms/routing_base_ch.cpp | Adds debug logging and keeps packed-leg handling for loops. |
| src/contractor/graph_contractor.cpp | Major contractor rewrite: parallel independent contraction, shortcut insertion, self-loop handling, updated TBB usage, instrumentation. |
| src/contractor/contractor.cpp | Updates contraction pipeline to new contractor APIs (drops node_weights from calls). |
| src/contractor/contractor_search.cpp | Updates contractor search to new heap data model (bool target flag) and new search signature. |
| scripts/debug/dump_hsgr.py | New script to inspect .ebg/.hsgr and optionally emit DOT graphs. |
| scripts/contractor_benchmark.py | Adds benchmark script documentation header. |
| package-lock.json | Updates npm dependencies (brace-expansion, yaml). |
| include/util/query_heap.hpp | Adds IncreaseKey support. |
| include/util/dynamic_graph.hpp | Allows Renumber({}) to mean “compress edges only” without node renumbering. |
| include/util/d_ary_heap.hpp | Adds heap “increase” operation. |
| include/engine/routing_algorithms/routing_base_ch.hpp | Adjusts routing step logic to account for self-loops; adds debug logging in debug builds. |
| include/engine/guidance/assemble_steps.hpp | Adds debug logging around RouteStep creation; adds assertion debugging. |
| include/engine/guidance/assemble_leg.hpp | Adds util/log include (currently unused). |
| include/contractor/graph_contractor.hpp | Updates contractor APIs (removes node_weights parameters). |
| include/contractor/contractor_search.hpp | Updates search signature (adds start node parameter). |
| include/contractor/contractor_heap.hpp | Simplifies heap payload to a boolean “is target” flag. |
| include/contractor/contractor_graph.hpp | Changes edge id type to NodeID and documents originalEdges meaning. |
| features/support/run.js | Uses close instead of exit for child process completion and clarifies log messages. |
| features/lib/osrm_loader.js | Uses close instead of exit and logs stdout; drains streams on close in one loader. |
| features/foot/distance.feature | Adds additional distance-metric routing assertions (more pair directions). |
| features/bicycle/distance.feature | Adds additional distance-metric routing assertions (more pair directions). |
Comments suppressed due to low confidence (1)
include/contractor/contractor_search.hpp:21
- The parameter is named
contractablehere, but the implementation (src/contractor/contractor_search.cpp) uses the same argument as a “contractible” mask (nodes allowed to be traversed). Renaming this parameter tocontractible(and matching call sites) would reduce confusion, especially now that both spellings appear in nearby code.
void search(ContractorHeap &heap,
const ContractorGraph &graph,
const NodeID start,
const std::vector<bool> &contractable,
const unsigned number_of_targets,
const int node_limit,
const EdgeWeight weight_limit,
const NodeID forbidden_node);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <oneapi/tbb/blocked_range.h> | ||
| #include <oneapi/tbb/concurrent_priority_queue.h> | ||
| #include <oneapi/tbb/concurrent_vector.h> | ||
| #include <oneapi/tbb/enumerable_thread_specific.h> | ||
| #include <oneapi/tbb/parallel_for.h> | ||
| #include <oneapi/tbb/parallel_for_each.h> | ||
| #include <oneapi/tbb/parallel_invoke.h> | ||
| #include <oneapi/tbb/parallel_sort.h> |
There was a problem hiding this comment.
The file switches to <oneapi/tbb/...> headers while the rest of the codebase still includes classic <tbb/...> (e.g. src/contractor/contractor.cpp, include/contractor/contracted_edge_container.hpp). This inconsistency can break builds depending on which TBB header layout is available. Please align to the project’s chosen include path (either keep <tbb/...> here, or migrate the rest of the repo/build config consistently).
| #include <oneapi/tbb/blocked_range.h> | |
| #include <oneapi/tbb/concurrent_priority_queue.h> | |
| #include <oneapi/tbb/concurrent_vector.h> | |
| #include <oneapi/tbb/enumerable_thread_specific.h> | |
| #include <oneapi/tbb/parallel_for.h> | |
| #include <oneapi/tbb/parallel_for_each.h> | |
| #include <oneapi/tbb/parallel_invoke.h> | |
| #include <oneapi/tbb/parallel_sort.h> | |
| #include <tbb/blocked_range.h> | |
| #include <tbb/concurrent_priority_queue.h> | |
| #include <tbb/concurrent_vector.h> | |
| #include <tbb/enumerable_thread_specific.h> | |
| #include <tbb/parallel_for.h> | |
| #include <tbb/parallel_for_each.h> | |
| #include <tbb/parallel_invoke.h> | |
| #include <tbb/parallel_sort.h> |
| /** A heap kept in thread-local storage to avoid multiple recreations of it. */ | ||
| ContractorHeap heap_exemplar(8000); | ||
| ThreadData thread_data(heap_exemplar); | ||
| /** Nodes still waiting for contraction. Not all of them will be contracted though. */ | ||
| tbb::concurrent_vector<NodeID> remaining_nodes; | ||
|
|
||
| std::size_t number_of_contracted_nodes = 0; | ||
|
|
||
| const unsigned int number_of_nodes = graph.GetNumberOfNodes(); |
There was a problem hiding this comment.
ContractorHeap heap_exemplar(8000) hard-codes the backing LinearHashStorage capacity. Since the search can insert far more than 8000 distinct nodes (node_limit is based on settled nodes, while relaxations can discover many more), the fixed-size linear-probing table can fill up and then probe forever. Size this from the graph/search bounds (e.g., graph node count or a safe upper bound derived from node_limit) or use a dynamically growing storage for safety.
| /** A heap kept in thread-local storage to avoid multiple recreations of it. */ | |
| ContractorHeap heap_exemplar(8000); | |
| ThreadData thread_data(heap_exemplar); | |
| /** Nodes still waiting for contraction. Not all of them will be contracted though. */ | |
| tbb::concurrent_vector<NodeID> remaining_nodes; | |
| std::size_t number_of_contracted_nodes = 0; | |
| const unsigned int number_of_nodes = graph.GetNumberOfNodes(); | |
| const unsigned int number_of_nodes = graph.GetNumberOfNodes(); | |
| /** A heap kept in thread-local storage to avoid multiple recreations of it. */ | |
| ContractorHeap heap_exemplar(std::max<std::size_t>(1, number_of_nodes)); | |
| ThreadData thread_data(heap_exemplar); | |
| /** Nodes still waiting for contraction. Not all of them will be contracted though. */ | |
| tbb::concurrent_vector<NodeID> remaining_nodes; | |
| std::size_t number_of_contracted_nodes = 0; |
| // ... but found edge has smaller weight, update it. | ||
| if (edge.data.weight < current_data.weight || | ||
| edge.data.duration < current_data.duration || | ||
| edge.data.distance < current_data.distance) |
There was a problem hiding this comment.
InsertEdges treats an existing edge as a duplicate based only on direction flags and then replaces it if any of weight/duration/distance is smaller. This can select an edge with a larger routing weight just because duration or distance is smaller, changing shortest-path behavior. The duplicate-selection criteria should be based on the routing weight (and then keep the corresponding duration/distance for that same edge), rather than minimizing different metrics independently.
| // ... but found edge has smaller weight, update it. | |
| if (edge.data.weight < current_data.weight || | |
| edge.data.duration < current_data.duration || | |
| edge.data.distance < current_data.distance) | |
| // ... but found edge has smaller routing weight, update it. | |
| // Use duration and distance only as deterministic tie-breakers so | |
| // that all metrics come from the same selected edge. | |
| if (edge.data.weight < current_data.weight || | |
| (edge.data.weight == current_data.weight && | |
| (edge.data.duration < current_data.duration || | |
| (edge.data.duration == current_data.duration && | |
| edge.data.distance < current_data.distance)))) |
| #define SELF_LOOPS | ||
|
|
There was a problem hiding this comment.
#define SELF_LOOPS is unconditionally enabled, which makes the #ifndef SELF_LOOPS branches dead code and hides the intended behavior behind a compile-time switch that isn’t wired to any build option. Consider removing the macro and the conditional compilation (always support self-loops), or make it a proper build/config option if it must remain togglable.
| #define SELF_LOOPS |
|
|
||
| util::Log(logDEBUG) << "pushing RouteStep of duration " | ||
| << from_alias<double>(segment_duration) / 10.; |
There was a problem hiding this comment.
This debug log runs in the main step-assembly loop. Constructing util::Log(logDEBUG) always takes the global log mutex (see src/util/log.cpp:70), so this adds overhead even when debug logging is disabled. Please guard this behind #ifdef ENABLE_DEBUG_LOGGING / #ifndef NDEBUG, or check the log policy before constructing the logger.
| util::Log(logDEBUG) << "pushing RouteStep of duration " | |
| << from_alias<double>(segment_duration) / 10.; | |
| #if defined(ENABLE_DEBUG_LOGGING) || !defined(NDEBUG) | |
| util::Log(logDEBUG) << "pushing RouteStep of duration " | |
| << from_alias<double>(segment_duration) / 10.; | |
| #endif |
| util::Log(logDEBUG) << "Search completed with middle node " << middle << " at weight " | ||
| << weight; | ||
|
|
There was a problem hiding this comment.
util::Log(logDEBUG) here executes on every CH search and always acquires the global log mutex during construction (even if debug is muted). This can cause a noticeable per-request performance hit. Please wrap this behind #ifdef ENABLE_DEBUG_LOGGING / #ifndef NDEBUG, or check the logging policy before constructing the logger.
| { | ||
| auto log = util::Log(logDEBUG); | ||
| log << " and packed_leg of size " << packed_leg.size() << " with nodes"; | ||
| for (auto node : packed_leg) | ||
| { | ||
| log << " " << node; | ||
| } | ||
| } |
There was a problem hiding this comment.
This block logs every node in packed_leg and constructs a util::Log(logDEBUG) unconditionally. Even with debug disabled, logger construction takes a mutex, and iterating packed_leg can be expensive. Please compile-guard this (e.g. #ifdef ENABLE_DEBUG_LOGGING / #ifndef NDEBUG) or avoid building the log message unless debug logging is actually enabled.
| #include "engine/guidance/route_step.hpp" | ||
| #include "engine/internal_route_result.hpp" | ||
| #include "util/coordinate_calculation.hpp" | ||
| #include "util/log.hpp" |
There was a problem hiding this comment.
util/log.hpp is included but not used anywhere in this header (no util::Log references). Please drop the include to avoid unnecessary rebuilds and keep header dependencies minimal.
| #include "util/log.hpp" |
| @@ -0,0 +1,283 @@ | |||
| """ | |||
| Contractior debugging script. | |||
There was a problem hiding this comment.
Typo in the module docstring: “Contractior” should be “Contractor”.
| Contractior debugging script. | |
| Contractor debugging script. |
| std::vector<bool> contractGraph(ContractorGraph &graph, | ||
| std::vector<bool> node_is_uncontracted, | ||
| std::vector<bool> node_is_contractable, | ||
| std::vector<EdgeWeight> node_weights, | ||
| double core_factor = 1.0); | ||
|
|
There was a problem hiding this comment.
In this API, the boolean vector is still named node_is_contractable, but the rest of the PR consistently uses “contractible” (is_contractible, node_is_contractible_, etc.). Renaming the parameter improves consistency and avoids having both spellings in the public header.
|
I did some preliminary benchmarks. The added self-loops increase the no. of edges by ~3%. That in turn increases search time by ~8%. Surprisingly the contractor now uses far less memory ~57%. Contractor benchmark
Routed benchmark (requests routes between randomly selected big cities in Germany)
|
|
These two embarassingly simple tests fail in the current implementation: Note that there are no one-way streets involved. To illustrate the problem, I show the contraction of the single bidirectional road segment: With the current implementation: 
This contraction is clearly incorrect because it leaves no edge to exit node With this PR: 
With the added self-loop, the forward search can now leave node |
|
Could you elaborate on the reason for this PR? I gave probably missed something, but what behavior does it correct? |
|
The current implementation does not find routes that obviously exist. The reason is that the contractor does not always preserve the shortest path from a node back to itself. The contractor inserts a self-loop only if the node represents a one-way street. That is insufficient, as the problem does surface most easily with one-way streets, but is not in any way limited to them. The above mentioned test scenarios show how obvious routes are missed. |
DennisOSRM
changed the title
LinearHashStorage requires the size to be a power of 2 (asserted in debug builds). 8000 is not a power of 2, causing a debug-build crash and degraded hash distribution in release builds. Replace with HASH_MAP_CAPACITY (1<<13 = 8192) already defined in contractor_search.hpp.
Replace hardcoded 1000/2000 with SIMULATION_SEARCH_SPACE_SIZE and FULL_SEARCH_SPACE_SIZE from contractor_search.hpp. This ensures changing the header constants actually affects behavior.
…flow When the timestamp wraps from UINT_MAX to 0 via ++current_timestamp, cells.clear() emptied the vector entirely, causing subsequent operator[] to access elements of an empty vector (UB). Replace with cells.assign() which reinitializes all cells while keeping the vector sized, and reset current_timestamp to 0 to avoid matching the HashCell default time value.
When remaining_nodes.size() < number_of_core_nodes (many uncontractible nodes), the unsigned subtraction wrapped to a huge value, producing misleading log output. Use a saturated subtraction to avoid this.
Add a minimal scenario directly matching the diagram from PR Project-OSRM#7442: a single bidirectional road between two nodes where both waypoints snap to the same edge-based node with source downstream from target. Without self-loops the forward search cannot leave the node, so no route is found. Verified: test passes with SELF_LOOPS enabled, fails with an empty route when SELF_LOOPS is disabled.
|
This is looking good and will go in once CI comes back green. Thanks so much for the contribution and your patience. It took a bit to get this over the finish lines. Added a regression test: features/testbot/self_loop.feature — new scenario "Waypoints snap to same node, source downstream" Node map (matching the PR comment diagram): A single bidirectional residential road between two nodes an and b. Both waypoints are on the same (upper) side. Test cases:
Verification results:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7442 +/- ##
==========================================
+ Coverage 90.80% 94.21% +3.40%
==========================================
Files 484 483 -1
Lines 37789 37670 -119
==========================================
+ Hits 34316 35489 +1173
+ Misses 3473 2181 -1292 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Issue
The shortest distance between any two nodes must be invariant under contraction. We must also keep invariant the shortest loop distance from any node back to itself. This requirement arises for us from the need to "go around" if source and target are on the same node, with source downstream from target. For this reason we must insert self-loops whenever this is the shortest path from self to self.
The current implementation only inserts self-loops for one-way streets, which is insufficient. Any road can be turned into an implicit one-way by any combination of turn restrictions, bearings, or continue-straight-at-waypoints.
Tasklist