Refactor shortestPathSearch to Parallel Delta-Stepping - #211
Open
nschimme wants to merge 5 commits into
Open
Conversation
This commit replaces the sequential Dijkstra implementation in `shortestpath.cpp` with a high-performance Parallel Delta-Stepping algorithm. Key improvements: - Parallelized relaxation of light and heavy edges using `thread_utils`. - Lock-free distance pruning using `std::atomic<double>`. - Sharded mutexes (1024 shards) with cache alignment to protect node metadata. - Thread-local improved node discovery to eliminate bucket contention. - Workload-aware adaptive parallelism (threshold: 256 nodes). - Optimized target lookup using a flat bitset-style vector. - Added `idealThreadCount()` helper to `thread_utils.h`. These changes significantly reduce search latency on multi-core systems while maintaining strict distance-ordered reporting of targets.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideRefactors the shortest path search from a sequential Dijkstra-style priority-queue traversal to a parallel Delta-Stepping algorithm with bucketed relaxation, lock-minimized shared state, and environment-aware threading, while preserving closest-first target reporting semantics. Sequence diagram for parallel DeltaStepping bucket processingsequenceDiagram
participant Caller as MapData_shortestPathSearch
participant Map as Map
participant ThreadUtils as thread_utils
participant Worker as WorkerThread
participant ShardArr as Shard_array
participant Recipient as ShortestPathRecipient
Caller->>Map: getRooms()
Map-->>Caller: ImmRoomIdSet
Caller->>Map: getRooms().last()
Map-->>Caller: max_room_id
Caller->>ThreadUtils: idealThreadCount()
ThreadUtils-->>Caller: numThreads
loop for each bucket_index
alt bucket not empty
Caller->>ThreadUtils: parallel_for_each_tl(light_nodes, ProgressCounter, relax_light, merge)
par for each WorkerThread on light edges
ThreadUtils->>Worker: dispatch u_id
Worker->>Map: getRoomHandle(u_id)
Worker->>Map: getExit(dir)
Worker->>Caller: getLength(exit, u_handle, v_handle)
Worker->>Caller: relax(v_id, new_dist, u_id, dir, dists, parents, lastdirs, locks, max_dist)
alt new_dist < dists[v]
Worker->>ShardArr: lock shard[v % SHARDS]
Worker->>Caller: update dists[v], parents[v], lastdirs[v]
Worker-->>ShardArr: unlock shard[v % SHARDS]
end
and merge thread locals
ThreadUtils-->>Caller: all_improved_light
end
Caller->>ThreadUtils: parallel_for_each_tl(bucket_nodes, ProgressCounter, relax_heavy, merge)
par for each WorkerThread on heavy edges
ThreadUtils->>Worker: dispatch u_id
Worker->>Map: getRoomHandle(u_id)
Worker->>Map: getExit(dir)
Worker->>Caller: getLength(exit, u_handle, v_handle)
Worker->>Caller: relax(v_id, new_dist, u_id, dir, dists, parents, lastdirs, locks, max_dist)
alt new_dist < dists[v]
Worker->>ShardArr: lock shard[v % SHARDS]
Worker->>Caller: update dists[v], parents[v], lastdirs[v]
Worker-->>ShardArr: unlock shard[v % SHARDS]
end
and merge thread locals
ThreadUtils-->>Caller: all_improved_heavy
end
Caller->>Caller: find targets_in_bucket and sort by dist
loop for each target in targets_in_bucket
Caller->>Caller: reconstruct path via parents and lastdirs
Caller->>Recipient: receiveShortestPath(map, result)
end
else bucket empty
Caller->>Caller: advance to next bucket_index
end
end
Class diagram for parallel DeltaStepping shortestPathSearch refactorclassDiagram
class MapData {
+shortestPathSearch(origin : RoomHandle, targets : RoomIdSet, recipient : ShortestPathRecipient, max_hits : int, max_dist : double) void
}
class ShortestPathRecipient {
<<interface>>
+receiveShortestPath(map : Map, result : ShortestPathResult) void
+~ShortestPathRecipient() void
}
class ShortestPathResult {
+id : RoomId
+dist : double
+path : vector~ExitDirEnum~
}
class Shard {
+mutex : mutex
}
class thread_utils {
+idealThreadCount() size_t
+parallel_for_each_tl_range(ThreadLocals, Container, ProgressCounter, Callback, MergeThreadLocals) void
+parallel_for_each_tl(ThreadLocals, Container, ProgressCounter, Callback, MergeThreadLocals) void
}
class ProgressCounter {
}
class RoomId {
+asUint32() uint32_t
}
class Map {
+getRooms() ImmRoomIdSet
+getRoomHandle(id : RoomId) RoomHandle
}
class ImmRoomIdSet {
+last() RoomId
}
class RoomHandle {
+getId() RoomId
+getExit(dir : ExitDirEnum) RawExit
}
class RawExit {
+outIsUnique() bool
+exitIsExit() bool
+getOutgoingSet() RoomIdSet
}
class RoomIdSet {
+first() RoomId
}
class ExitDirEnum {
}
MapData --> Map : uses
MapData --> Shard : uses
MapData --> ShortestPathRecipient : notifies
MapData --> ShortestPathResult : constructs
MapData --> thread_utils : uses
MapData --> ProgressCounter : uses
MapData --> RoomId : uses
MapData --> RoomHandle : uses
MapData --> RawExit : uses
ShortestPathResult --> RoomId : has
ShortestPathResult --> ExitDirEnum : path elements
Map --> ImmRoomIdSet : owns
Map --> RoomHandle : returns
ImmRoomIdSet --> RoomId : returns
RoomHandle --> RawExit : returns
RawExit --> RoomIdSet : outgoing
Shard --> mutex : contains
thread_utils --> ProgressCounter : uses
thread_utils --> std_thread : hardware_concurrency
class std_thread {
+hardware_concurrency() unsigned int
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new array-based state (
dists,parents,lastdirs,is_target) assumesRoomId::asUint32()is reasonably dense up tomap.getRooms().last(); ifRoomIdvalues can be sparse or have a large maximum, consider introducing a compact ID mapping to avoid potentially very large allocations. - The light and heavy relaxation lambdas (
relax_lightandrelax_heavy) duplicate most of their traversal logic; factoring the shared code into a single helper that branches only on theweight <= DELTAcondition would reduce maintenance overhead and the risk of the two paths diverging in behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new array-based state (`dists`, `parents`, `lastdirs`, `is_target`) assumes `RoomId::asUint32()` is reasonably dense up to `map.getRooms().last()`; if `RoomId` values can be sparse or have a large maximum, consider introducing a compact ID mapping to avoid potentially very large allocations.
- The light and heavy relaxation lambdas (`relax_light` and `relax_heavy`) duplicate most of their traversal logic; factoring the shared code into a single helper that branches only on the `weight <= DELTA` condition would reduce maintenance overhead and the risk of the two paths diverging in behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor-sp #211 +/- ##
==============================================
Coverage ? 25.37%
==============================================
Files ? 519
Lines ? 43165
Branches ? 4717
==============================================
Hits ? 10954
Misses ? 32211
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This commit refactors the shortest path search algorithm from sequential Dijkstra to Parallel Delta-Stepping (with delta=2.0) to reduce latency. Key improvements: - Implemented Parallel Delta-Stepping with lock-free distance pruning using std::atomic<float>. - Used sharded, cache-aligned mutexes (1024 shards) to protect parent and direction updates, minimizing contention. - Parallelized relaxation of light and heavy edges using thread_utils::parallel_for_each_tl. - Optimized target lookup with a flat vector-based bitset (O(1)). - Introduced a parallelism threshold (256 nodes) to avoid overhead on small searches. - Added thread_utils::idealThreadCount() for portable thread discovery. - Reduced memory overhead by replacing high-level containers with flat arrays for search state.
This commit refactors the shortest path search algorithm from sequential Dijkstra to Parallel Delta-Stepping (with delta=2.0) to reduce latency. It also includes necessary formatting fixes to satisfy CI. Key improvements: - Implemented Parallel Delta-Stepping with lock-free distance pruning using std::atomic<float>. - Used sharded, cache-aligned mutexes (1024 shards) to protect parent and direction updates, minimizing contention. - Parallelized relaxation of light and heavy edges using thread_utils::parallel_for_each_tl. - Optimized target lookup with a flat vector-based bitset (O(1)). - Introduced a parallelism threshold (256 nodes) to avoid overhead on small searches. - Added thread_utils::idealThreadCount() for portable thread discovery. - Reduced memory overhead by replacing high-level containers with flat arrays for search state. - Applied clang-format to satisfy CI requirements.
Refactored the shortest path search algorithm from sequential Dijkstra to Parallel Delta-Stepping (delta=2.0) to reduce latency as requested. Key improvements: - Implemented Parallel Delta-Stepping using thread_utils::parallel_for_each_tl. - Used std::atomic<float> for lock-free distance pruning. - Implemented sharded cache-aligned mutexes (1024 shards) to protect concurrent updates to search state. - Utilized idiomatic data structures: RoomIdSet for targets and IndexedVector for parents and directions. - Scaled concurrency using thread_utils::idealThreadCount(). - Reduced precision to float for better performance and memory bandwidth. - Applied explicit lambda captures and followed repository formatting rules.
Refactored the shortest path search algorithm from sequential Dijkstra to Parallel Delta-Stepping (delta=2.0). Key improvements: - Implemented Parallel Delta-Stepping using thread_utils::parallel_for_each_tl. - Introduced Bucket and BucketList types for stronger typing of nodes. - Used std::atomic<float> for lock-free distance pruning. - Implemented sharded cache-aligned mutexes (1024 shards) for low-contention concurrent updates. - Explicitly documented the use of std::make_unique for non-movable types (atomic, mutex) as required. - Utilized idiomatic repository data structures: RoomIdSet and IndexedVector. - Switched to float for better performance and reduced memory bandwidth. - Applied clang-format and explicit lambda captures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I have refactored the shortest path search algorithm from a sequential Dijkstra approach to a high-performance Parallel Delta-Stepping implementation. This refactor targets the 1-second latency reported by the user by leveraging multiple CPU cores and optimizing data access patterns.
Technical Highlights:
std::atomic<double>, allowing threads to immediately skip sub-optimal paths without acquiring locks.alignas(64)) to prevent false sharing and minimize lock contention.std::set-based target lookups with a flatstd::vector<uint8_t>forthread_utils::idealThreadCount()to provide a consistent way for parallel algorithms to scale based on the execution environment.Verified the implementation through compilation in the main target and passing existing unit tests.
PR created automatically by Jules for task 5298116093134269607 started by @nschimme
Summary by Sourcery
Refactor the shortest path search to a parallel delta-stepping algorithm with bucketed processing and improved multi-threaded performance characteristics.
Enhancements: