From 774992c246a0c2576534ce57de721f529ebb193c Mon Sep 17 00:00:00 2001 From: RoosterDragon Date: Fri, 21 Aug 2015 21:27:20 +0100 Subject: [PATCH 1/3] Cache only unit paths in the pathfinder. The path caching works on the assumption that the time saved from reusing a cached path outweights the cost of caching it in the first place. For unit paths, this assumption holds. For path searchs, we spend more time caching them then we save when we get to reuse these cached paths. This is because they are reused less often, and calculating their key is more expensive in comparison. --- OpenRA.Mods.Common/OpenRA.Mods.Common.csproj | 2 +- .../Pathfinder/BasePathSearch.cs | 33 ------------------ ...cs => PathFinderUnitPathCacheDecorator.cs} | 34 +++---------------- OpenRA.Mods.Common/Traits/World/PathFinder.cs | 2 +- 4 files changed, 7 insertions(+), 64 deletions(-) rename OpenRA.Mods.Common/Pathfinder/{PathFinderCacheDecorator.cs => PathFinderUnitPathCacheDecorator.cs} (71%) diff --git a/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj b/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj index 3314d73807..479b58949b 100644 --- a/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj +++ b/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj @@ -508,7 +508,7 @@ - + diff --git a/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs b/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs index 57fb3c3cad..13c8c0e79d 100644 --- a/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs +++ b/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs @@ -18,8 +18,6 @@ namespace OpenRA.Mods.Common.Pathfinder { public interface IPathSearch { - string Id { get; } - /// /// The Graph used by the A* /// @@ -71,36 +69,6 @@ namespace OpenRA.Mods.Common.Pathfinder { public IGraph Graph { get; set; } - // The Id of a Pathsearch is computed by its properties. - // So two PathSearch instances with the same parameters will - // Compute the same Id. This is used for caching purposes. - public string Id - { - get - { - if (string.IsNullOrEmpty(id)) - { - var builder = new StringBuilder(); - builder.Append(Graph.Actor.ActorID); - while (!startPoints.Empty) - { - var startpoint = startPoints.Pop(); - builder.Append(startpoint.X); - builder.Append(startpoint.Y); - builder.Append(Graph[startpoint].EstimatedTotal); - } - - builder.Append(Graph.InReverse); - if (Graph.IgnoredActor != null) builder.Append(Graph.IgnoredActor.ActorID); - builder.Append(Graph.LaneBias); - - id = builder.ToString(); - } - - return id; - } - } - public IPriorityQueue OpenQueue { get; protected set; } public abstract IEnumerable> Considered { get; } @@ -108,7 +76,6 @@ namespace OpenRA.Mods.Common.Pathfinder public Player Owner { get { return Graph.Actor.Owner; } } public int MaxCost { get; protected set; } public bool Debug { get; set; } - string id; protected Func heuristic; protected Func isGoal; diff --git a/OpenRA.Mods.Common/Pathfinder/PathFinderCacheDecorator.cs b/OpenRA.Mods.Common/Pathfinder/PathFinderUnitPathCacheDecorator.cs similarity index 71% rename from OpenRA.Mods.Common/Pathfinder/PathFinderCacheDecorator.cs rename to OpenRA.Mods.Common/Pathfinder/PathFinderUnitPathCacheDecorator.cs index f4fad6cf99..1aeaf768ac 100644 --- a/OpenRA.Mods.Common/Pathfinder/PathFinderCacheDecorator.cs +++ b/OpenRA.Mods.Common/Pathfinder/PathFinderUnitPathCacheDecorator.cs @@ -16,14 +16,14 @@ using OpenRA.Traits; namespace OpenRA.Mods.Common.Pathfinder { /// - /// A decorator used to cache the pathfinder (Decorator design pattern) + /// A decorator used to cache FindUnitPath and FindUnitPathToRange (Decorator design pattern) /// - public class PathFinderCacheDecorator : IPathFinder + public class PathFinderUnitPathCacheDecorator : IPathFinder { readonly IPathFinder pathFinder; readonly ICacheStorage> cacheStorage; - public PathFinderCacheDecorator(IPathFinder pathFinder, ICacheStorage> cacheStorage) + public PathFinderUnitPathCacheDecorator(IPathFinder pathFinder, ICacheStorage> cacheStorage) { this.pathFinder = pathFinder; this.cacheStorage = cacheStorage; @@ -68,37 +68,13 @@ namespace OpenRA.Mods.Common.Pathfinder public List FindPath(IPathSearch search) { using (new PerfSample("Pathfinder")) - { - var key = "FindPath" + search.Id; - var cachedPath = cacheStorage.Retrieve(key); - - if (cachedPath != null) - return cachedPath; - - var pb = pathFinder.FindPath(search); - - cacheStorage.Store(key, pb); - - return pb; - } + return pathFinder.FindPath(search); } public List FindBidiPath(IPathSearch fromSrc, IPathSearch fromDest) { using (new PerfSample("Pathfinder")) - { - var key = "FindBidiPath" + fromSrc.Id + fromDest.Id; - var cachedPath = cacheStorage.Retrieve(key); - - if (cachedPath != null) - return cachedPath; - - var pb = pathFinder.FindBidiPath(fromSrc, fromDest); - - cacheStorage.Store(key, pb); - - return pb; - } + return pathFinder.FindBidiPath(fromSrc, fromDest); } } } diff --git a/OpenRA.Mods.Common/Traits/World/PathFinder.cs b/OpenRA.Mods.Common/Traits/World/PathFinder.cs index 70e71fcfc6..6524d67775 100644 --- a/OpenRA.Mods.Common/Traits/World/PathFinder.cs +++ b/OpenRA.Mods.Common/Traits/World/PathFinder.cs @@ -22,7 +22,7 @@ namespace OpenRA.Mods.Common.Traits { public object Create(ActorInitializer init) { - return new PathFinderCacheDecorator(new PathFinder(init.World), new PathCacheStorage(init.World)); + return new PathFinderUnitPathCacheDecorator(new PathFinder(init.World), new PathCacheStorage(init.World)); } } From 77923a27c1b922f4291ee960949f55202f49eaf1 Mon Sep 17 00:00:00 2001 From: RoosterDragon Date: Thu, 20 Aug 2015 23:26:35 +0100 Subject: [PATCH 2/3] Tweak IPathSearch to avoid exposing the OpenQueue directly. --- OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs | 11 ++++------- OpenRA.Mods.Common/Traits/World/PathFinder.cs | 4 ++-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs b/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs index 13c8c0e79d..8fba8f3e09 100644 --- a/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs +++ b/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs @@ -23,11 +23,6 @@ namespace OpenRA.Mods.Common.Pathfinder /// IGraph Graph { get; } - /// - /// The open queue where nodes that are worth to consider are stored by their estimator - /// - IPriorityQueue OpenQueue { get; } - /// /// Stores the analyzed nodes by the expand function /// @@ -62,6 +57,7 @@ namespace OpenRA.Mods.Common.Pathfinder /// Whether the location is a target bool IsTarget(CPos location); + bool CanExpand { get; } CPos Expand(); } @@ -69,7 +65,7 @@ namespace OpenRA.Mods.Common.Pathfinder { public IGraph Graph { get; set; } - public IPriorityQueue OpenQueue { get; protected set; } + protected IPriorityQueue OpenQueue { get; private set; } public abstract IEnumerable> Considered { get; } @@ -84,7 +80,7 @@ namespace OpenRA.Mods.Common.Pathfinder // points considered and their Heuristics to reach // the target. It pretty match identifies, in conjunction of the Actor, // a deterministic set of calculations - protected IPriorityQueue startPoints; + protected readonly IPriorityQueue startPoints; protected BasePathSearch(IGraph graph) { @@ -165,6 +161,7 @@ namespace OpenRA.Mods.Common.Pathfinder return isGoal(location); } + public bool CanExpand { get { return !OpenQueue.Empty; } } public abstract CPos Expand(); } } diff --git a/OpenRA.Mods.Common/Traits/World/PathFinder.cs b/OpenRA.Mods.Common/Traits/World/PathFinder.cs index 6524d67775..405f2b2a28 100644 --- a/OpenRA.Mods.Common/Traits/World/PathFinder.cs +++ b/OpenRA.Mods.Common/Traits/World/PathFinder.cs @@ -121,7 +121,7 @@ namespace OpenRA.Mods.Common.Traits List path = null; - while (!search.OpenQueue.Empty) + while (search.CanExpand) { var p = search.Expand(); if (search.IsTarget(p)) @@ -156,7 +156,7 @@ namespace OpenRA.Mods.Common.Traits fromDest.Debug = true; } - while (!fromSrc.OpenQueue.Empty && !fromDest.OpenQueue.Empty) + while (fromSrc.CanExpand && fromDest.CanExpand) { // make some progress on the first search var p = fromSrc.Expand(); From ac55c5bf0966a369ef52ecb0233bceda160b6844 Mon Sep 17 00:00:00 2001 From: RoosterDragon Date: Thu, 20 Aug 2015 23:36:44 +0100 Subject: [PATCH 3/3] Fix pathfinding using PriorityQueue incorrectly. By providing a comparer that could change over time (as estimated costs on the graph were updated), this meant the priority queue could have its heap property invalidated and thus not maintain a correct ordering. Instead we store elements into the queue with their estimations at the time. This preserves the heap property and thus ensures the queue returns properly ordered results, although it may contain out of date estimations. This also improves performance. The fixed comparer need not perform expensive lookups into the graph, but can instead use the readily available value. This speeds up adds and removes on the queue significantly. --- .../Pathfinder/BasePathSearch.cs | 8 ++++---- OpenRA.Mods.Common/Pathfinder/CellInfo.cs | 18 ------------------ OpenRA.Mods.Common/Pathfinder/PathGraph.cs | 12 ++++++++++++ OpenRA.Mods.Common/Pathfinder/PathSearch.cs | 15 +++++++++------ 4 files changed, 25 insertions(+), 28 deletions(-) diff --git a/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs b/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs index 8fba8f3e09..fab51285a3 100644 --- a/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs +++ b/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs @@ -65,7 +65,7 @@ namespace OpenRA.Mods.Common.Pathfinder { public IGraph Graph { get; set; } - protected IPriorityQueue OpenQueue { get; private set; } + protected IPriorityQueue OpenQueue { get; private set; } public abstract IEnumerable> Considered { get; } @@ -80,13 +80,13 @@ namespace OpenRA.Mods.Common.Pathfinder // points considered and their Heuristics to reach // the target. It pretty match identifies, in conjunction of the Actor, // a deterministic set of calculations - protected readonly IPriorityQueue startPoints; + protected readonly IPriorityQueue StartPoints; protected BasePathSearch(IGraph graph) { Graph = graph; - OpenQueue = new PriorityQueue(new PositionComparer(Graph)); - startPoints = new PriorityQueue(new PositionComparer(Graph)); + OpenQueue = new PriorityQueue(GraphConnection.ConnectionCostComparer); + StartPoints = new PriorityQueue(GraphConnection.ConnectionCostComparer); Debug = false; MaxCost = 0; } diff --git a/OpenRA.Mods.Common/Pathfinder/CellInfo.cs b/OpenRA.Mods.Common/Pathfinder/CellInfo.cs index 57f3e9329e..a8112bc343 100644 --- a/OpenRA.Mods.Common/Pathfinder/CellInfo.cs +++ b/OpenRA.Mods.Common/Pathfinder/CellInfo.cs @@ -57,22 +57,4 @@ namespace OpenRA.Mods.Common.Pathfinder EstimatedTotal = estimatedTotal; } } - - /// - /// Compares two nodes according to their estimations - /// - public class PositionComparer : IComparer - { - readonly IGraph graph; - - public PositionComparer(IGraph graph) - { - this.graph = graph; - } - - public int Compare(CPos x, CPos y) - { - return Math.Sign(graph[x].EstimatedTotal - graph[y].EstimatedTotal); - } - } } diff --git a/OpenRA.Mods.Common/Pathfinder/PathGraph.cs b/OpenRA.Mods.Common/Pathfinder/PathGraph.cs index 3204b5282e..268e1331c2 100644 --- a/OpenRA.Mods.Common/Pathfinder/PathGraph.cs +++ b/OpenRA.Mods.Common/Pathfinder/PathGraph.cs @@ -47,6 +47,18 @@ namespace OpenRA.Mods.Common.Pathfinder public struct GraphConnection { + public static readonly CostComparer ConnectionCostComparer = CostComparer.Instance; + + public sealed class CostComparer : IComparer + { + public static readonly CostComparer Instance = new CostComparer(); + CostComparer() { } + public int Compare(GraphConnection x, GraphConnection y) + { + return x.Cost.CompareTo(y.Cost); + } + } + public readonly CPos Destination; public readonly int Cost; diff --git a/OpenRA.Mods.Common/Pathfinder/PathSearch.cs b/OpenRA.Mods.Common/Pathfinder/PathSearch.cs index 254317d030..c4f5355f7d 100644 --- a/OpenRA.Mods.Common/Pathfinder/PathSearch.cs +++ b/OpenRA.Mods.Common/Pathfinder/PathSearch.cs @@ -84,9 +84,11 @@ namespace OpenRA.Mods.Common.Pathfinder protected override void AddInitialCell(CPos location) { - Graph[location] = new CellInfo(0, heuristic(location), location, CellStatus.Open); - OpenQueue.Add(location); - startPoints.Add(location); + var cost = heuristic(location); + Graph[location] = new CellInfo(0, cost, location, CellStatus.Open); + var connection = new GraphConnection(location, cost); + OpenQueue.Add(connection); + StartPoints.Add(connection); considered.AddLast(new Pair(location, 0)); } @@ -99,7 +101,7 @@ namespace OpenRA.Mods.Common.Pathfinder /// The most promising node of the iteration public override CPos Expand() { - var currentMinNode = OpenQueue.Pop(); + var currentMinNode = OpenQueue.Pop().Destination; var currentCell = Graph[currentMinNode]; Graph[currentMinNode] = new CellInfo(currentCell.CostSoFar, currentCell.EstimatedTotal, currentCell.PreviousPos, CellStatus.Closed); @@ -128,10 +130,11 @@ namespace OpenRA.Mods.Common.Pathfinder else hCost = heuristic(neighborCPos); - Graph[neighborCPos] = new CellInfo(gCost, gCost + hCost, currentMinNode, CellStatus.Open); + var estimatedCost = gCost + hCost; + Graph[neighborCPos] = new CellInfo(gCost, estimatedCost, currentMinNode, CellStatus.Open); if (neighborCell.Status != CellStatus.Open) - OpenQueue.Add(neighborCPos); + OpenQueue.Add(new GraphConnection(neighborCPos, estimatedCost)); if (Debug) {