From 519be4374c2fbd8710669387be57be66a8ee1f9c Mon Sep 17 00:00:00 2001 From: RoosterDragon Date: Thu, 13 Aug 2015 23:38:23 +0100 Subject: [PATCH] Fixed pooling of layers used for pathfinding. The previous implementation: - Was failing to dispose of pooled layers. - Was using a finalizer to allow undisposed layers to be reused. This means all pooled layers are kept alive indefinitely until the map changes. If the finalizer is slow for any reason then the pathfiinder will allocate new layers when the pool runs out. Since these new layers are eventually stuffed back into the pool when the finalizer does run, this can theoretically leak unbounded memory until the pool goes out of scope. In practice it would leak tens of megabytes. The new implementation ensures layers are disposed and pooled correctly to allow proper memory reuse. It also introduces some safeguards against memory leaks: - A cap is set on the number of pooled layers. If more concurrent layers are needed than this, then the excess layers will not be pooled but instead be allowed to be garbage collected. - No finalizer. An implementation that fails to call dispose simply allows the layer to be garbage collected instead. --- .../Activities/FindResources.cs | 10 +- OpenRA.Mods.Common/Activities/Move/Move.cs | 18 ++- .../Activities/Move/MoveAdjacentTo.cs | 7 +- OpenRA.Mods.Common/OpenRA.Mods.Common.csproj | 2 +- .../Pathfinder/BasePathSearch.cs | 15 ++- .../Pathfinder/CellInfoLayerManager.cs | 111 ------------------ .../Pathfinder/CellInfoLayerPool.cs | 78 ++++++++++++ OpenRA.Mods.Common/Pathfinder/PathGraph.cs | 30 ++--- OpenRA.Mods.Common/Pathfinder/PathSearch.cs | 15 ++- OpenRA.Mods.Common/Traits/Harvester.cs | 27 +++-- OpenRA.Mods.Common/Traits/World/PathFinder.cs | 15 ++- 11 files changed, 158 insertions(+), 170 deletions(-) delete mode 100644 OpenRA.Mods.Common/Pathfinder/CellInfoLayerManager.cs create mode 100644 OpenRA.Mods.Common/Pathfinder/CellInfoLayerPool.cs diff --git a/OpenRA.Mods.Common/Activities/FindResources.cs b/OpenRA.Mods.Common/Activities/FindResources.cs index fc35ca2d95..a173de2a8c 100644 --- a/OpenRA.Mods.Common/Activities/FindResources.cs +++ b/OpenRA.Mods.Common/Activities/FindResources.cs @@ -123,8 +123,10 @@ namespace OpenRA.Mods.Common.Activities var searchRadius = harv.LastOrderLocation.HasValue ? harvInfo.SearchFromOrderRadius : harvInfo.SearchFromProcRadius; var searchRadiusSquared = searchRadius * searchRadius; + // Find any harvestable resources: var passable = (uint)mobileInfo.GetMovementClass(self.World.TileSet); - var search = PathSearch.Search(self.World, mobileInfo, self, true, + List path; + using (var search = PathSearch.Search(self.World, mobileInfo, self, true, loc => domainIndex.IsPassable(self.Location, loc, passable) && self.CanHarvestAt(loc, resLayer, harvInfo, territory)) .WithCustomCost(loc => { @@ -135,10 +137,8 @@ namespace OpenRA.Mods.Common.Activities return 0; }) .FromPoint(self.Location) - .FromPoint(searchFromLoc); - - // Find any harvestable resources: - var path = pathFinder.FindPath(search); + .FromPoint(searchFromLoc)) + path = pathFinder.FindPath(search); if (path.Count > 0) return path[0]; diff --git a/OpenRA.Mods.Common/Activities/Move/Move.cs b/OpenRA.Mods.Common/Activities/Move/Move.cs index e5a704d33a..5c8ea5b1a0 100644 --- a/OpenRA.Mods.Common/Activities/Move/Move.cs +++ b/OpenRA.Mods.Common/Activities/Move/Move.cs @@ -46,9 +46,14 @@ namespace OpenRA.Mods.Common.Activities moveDisablers = self.TraitsImplementing().ToArray(); getPath = () => - self.World.WorldActor.Trait().FindPath( + { + List path; + using (var search = PathSearch.FromPoint(self.World, mobile.Info, self, mobile.ToCell, destination, false) - .WithoutLaneBias()); + .WithoutLaneBias()) + path = self.World.WorldActor.Trait().FindPath(search); + return path; + }; this.destination = destination; this.nearEnough = WDist.Zero; } @@ -85,9 +90,14 @@ namespace OpenRA.Mods.Common.Activities moveDisablers = self.TraitsImplementing().ToArray(); getPath = () => - self.World.WorldActor.Trait().FindPath( + { + List path; + using (var search = PathSearch.FromPoint(self.World, mobile.Info, self, mobile.ToCell, destination, false) - .WithIgnoredActor(ignoredActor)); + .WithIgnoredActor(ignoredActor)) + path = self.World.WorldActor.Trait().FindPath(search); + return path; + }; this.destination = destination; this.nearEnough = WDist.Zero; diff --git a/OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs b/OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs index db9624cc5e..90e9b519eb 100644 --- a/OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs +++ b/OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs @@ -148,10 +148,9 @@ namespace OpenRA.Mods.Common.Activities if (!searchCells.Any()) return NoPath; - var fromSrc = PathSearch.FromPoints(self.World, mobile.Info, self, searchCells, loc, true); - var fromDest = PathSearch.FromPoint(self.World, mobile.Info, self, loc, targetPosition, true).Reverse(); - - return pathFinder.FindBidiPath(fromSrc, fromDest); + using (var fromSrc = PathSearch.FromPoints(self.World, mobile.Info, self, searchCells, loc, true)) + using (var fromDest = PathSearch.FromPoint(self.World, mobile.Info, self, loc, targetPosition, true).Reverse()) + return pathFinder.FindBidiPath(fromSrc, fromDest); } public override IEnumerable GetTargets(Actor self) diff --git a/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj b/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj index 0af1d6857c..62df623ae2 100644 --- a/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj +++ b/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj @@ -503,7 +503,7 @@ - + diff --git a/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs b/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs index fab51285a3..ff680d74aa 100644 --- a/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs +++ b/OpenRA.Mods.Common/Pathfinder/BasePathSearch.cs @@ -11,12 +11,11 @@ using System; using System.Collections.Generic; using System.Text; -using OpenRA.Mods.Common.Traits; using OpenRA.Primitives; namespace OpenRA.Mods.Common.Pathfinder { - public interface IPathSearch + public interface IPathSearch : IDisposable { /// /// The Graph used by the A* @@ -163,5 +162,17 @@ namespace OpenRA.Mods.Common.Pathfinder public bool CanExpand { get { return !OpenQueue.Empty; } } public abstract CPos Expand(); + + protected virtual void Dispose(bool disposing) + { + if (disposing) + Graph.Dispose(); + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } } } diff --git a/OpenRA.Mods.Common/Pathfinder/CellInfoLayerManager.cs b/OpenRA.Mods.Common/Pathfinder/CellInfoLayerManager.cs deleted file mode 100644 index a78a783cf1..0000000000 --- a/OpenRA.Mods.Common/Pathfinder/CellInfoLayerManager.cs +++ /dev/null @@ -1,111 +0,0 @@ -#region Copyright & License Information -/* - * Copyright 2007-2015 The OpenRA Developers (see AUTHORS) - * This file is part of OpenRA, which is free software. It is made - * available to you under the terms of the GNU General Public License - * as published by the Free Software Foundation. For more information, - * see COPYING. - */ -#endregion - -using System.Collections.Generic; -using System.Drawing; -using OpenRA.Mods.Common.Traits; - -namespace OpenRA.Mods.Common.Pathfinder -{ - public interface ICellInfoLayerManager - { - /// - /// Gets a CellLayer of Nodes from the pool - /// - CellLayer GetFromPool(); - - /// - /// Puts a CellLayer into the pool - /// - void PutBackIntoPool(CellLayer ci); - - /// - /// Creates (or obtains from the pool) a CellLayer given a map - /// - CellLayer NewLayer(Map map); - } - - public sealed class CellInfoLayerManager : ICellInfoLayerManager - { - readonly Queue> cellInfoPool = new Queue>(); - readonly object defaultCellInfoLayerSync = new object(); - CellLayer defaultCellInfoLayer; - - static ICellInfoLayerManager instance = new CellInfoLayerManager(); - - public static ICellInfoLayerManager Instance - { - get - { - return instance; - } - } - - public static void SetInstance(ICellInfoLayerManager cellInfoLayerManager) - { - instance = cellInfoLayerManager; - } - - public CellLayer GetFromPool() - { - lock (cellInfoPool) - return cellInfoPool.Dequeue(); - } - - public void PutBackIntoPool(CellLayer ci) - { - lock (cellInfoPool) - cellInfoPool.Enqueue(ci); - } - - public CellLayer NewLayer(Map map) - { - CellLayer result = null; - var mapSize = new Size(map.MapSize.X, map.MapSize.Y); - - // HACK: Uses a static cache so that double-ended searches (which have two PathSearch instances) - // can implicitly share data. The PathFinder should allocate the CellInfo array and pass it - // explicitly to the things that need to share it. - while (cellInfoPool.Count > 0) - { - var cellInfo = GetFromPool(); - if (cellInfo.Size != mapSize || cellInfo.Shape != map.TileShape) - { - Log.Write("debug", "Discarding old pooled CellInfo of wrong size."); - continue; - } - - result = cellInfo; - break; - } - - if (result == null) - result = new CellLayer(map); - - lock (defaultCellInfoLayerSync) - { - if (defaultCellInfoLayer == null || - defaultCellInfoLayer.Size != mapSize || - defaultCellInfoLayer.Shape != map.TileShape) - { - defaultCellInfoLayer = - CellLayer.CreateInstance( - mpos => new CellInfo(int.MaxValue, int.MaxValue, mpos.ToCPos(map), CellStatus.Unvisited), - new Size(map.MapSize.X, map.MapSize.Y), - map.TileShape); - } - - result.CopyValuesFrom(defaultCellInfoLayer); - } - - return result; - } - } -} \ No newline at end of file diff --git a/OpenRA.Mods.Common/Pathfinder/CellInfoLayerPool.cs b/OpenRA.Mods.Common/Pathfinder/CellInfoLayerPool.cs new file mode 100644 index 0000000000..1bfb39a6c0 --- /dev/null +++ b/OpenRA.Mods.Common/Pathfinder/CellInfoLayerPool.cs @@ -0,0 +1,78 @@ +#region Copyright & License Information +/* + * Copyright 2007-2015 The OpenRA Developers (see AUTHORS) + * This file is part of OpenRA, which is free software. It is made + * available to you under the terms of the GNU General Public License + * as published by the Free Software Foundation. For more information, + * see COPYING. + */ +#endregion + +using System; +using System.Collections.Generic; +using System.Drawing; + +namespace OpenRA.Mods.Common.Pathfinder +{ + sealed class CellInfoLayerPool + { + const int MaxPoolSize = 4; + readonly Stack> pool = new Stack>(MaxPoolSize); + readonly CellLayer defaultLayer; + + public CellInfoLayerPool(Map map) + { + defaultLayer = + CellLayer.CreateInstance( + mpos => new CellInfo(int.MaxValue, int.MaxValue, mpos.ToCPos(map), CellStatus.Unvisited), + new Size(map.MapSize.X, map.MapSize.Y), + map.TileShape); + } + + public PooledCellInfoLayer Get() + { + return new PooledCellInfoLayer(this); + } + + CellLayer GetLayer() + { + CellLayer layer = null; + lock (pool) + if (pool.Count > 0) + layer = pool.Pop(); + + if (layer == null) + layer = new CellLayer(defaultLayer.Shape, defaultLayer.Size); + layer.CopyValuesFrom(defaultLayer); + return layer; + } + + void ReturnLayer(CellLayer layer) + { + lock (pool) + if (pool.Count < MaxPoolSize) + pool.Push(layer); + } + + public class PooledCellInfoLayer : IDisposable + { + public CellLayer Layer { get; private set; } + CellInfoLayerPool layerPool; + + public PooledCellInfoLayer(CellInfoLayerPool layerPool) + { + this.layerPool = layerPool; + Layer = layerPool.GetLayer(); + } + + public void Dispose() + { + if (Layer == null) + return; + layerPool.ReturnLayer(Layer); + Layer = null; + layerPool = null; + } + } + } +} \ No newline at end of file diff --git a/OpenRA.Mods.Common/Pathfinder/PathGraph.cs b/OpenRA.Mods.Common/Pathfinder/PathGraph.cs index 268e1331c2..338cce57fa 100644 --- a/OpenRA.Mods.Common/Pathfinder/PathGraph.cs +++ b/OpenRA.Mods.Common/Pathfinder/PathGraph.cs @@ -69,7 +69,7 @@ namespace OpenRA.Mods.Common.Pathfinder } } - public class PathGraph : IGraph + sealed class PathGraph : IGraph { public Actor Actor { get; private set; } public World World { get; private set; } @@ -82,11 +82,13 @@ namespace OpenRA.Mods.Common.Pathfinder readonly CellConditions checkConditions; readonly MobileInfo mobileInfo; readonly MobileInfo.WorldMovementInfo worldMovementInfo; + readonly CellInfoLayerPool.PooledCellInfoLayer pooledLayer; CellLayer cellInfo; - public PathGraph(CellLayer cellInfo, MobileInfo mobileInfo, Actor actor, World world, bool checkForBlocked) + public PathGraph(CellInfoLayerPool layerPool, MobileInfo mobileInfo, Actor actor, World world, bool checkForBlocked) { - this.cellInfo = cellInfo; + pooledLayer = layerPool.Get(); + cellInfo = pooledLayer.Layer; World = world; this.mobileInfo = mobileInfo; worldMovementInfo = mobileInfo.GetWorldMovementInfo(world); @@ -174,26 +176,16 @@ namespace OpenRA.Mods.Common.Pathfinder return cellCost; } - bool disposed; - public void Dispose() - { - if (disposed) - return; - - disposed = true; - - CellInfoLayerManager.Instance.PutBackIntoPool(cellInfo); - cellInfo = null; - - GC.SuppressFinalize(this); - } - - ~PathGraph() { Dispose(); } - public CellInfo this[CPos pos] { get { return cellInfo[pos]; } set { cellInfo[pos] = value; } } + + public void Dispose() + { + pooledLayer.Dispose(); + cellInfo = null; + } } } diff --git a/OpenRA.Mods.Common/Pathfinder/PathSearch.cs b/OpenRA.Mods.Common/Pathfinder/PathSearch.cs index c4f5355f7d..b7a6cd50ff 100644 --- a/OpenRA.Mods.Common/Pathfinder/PathSearch.cs +++ b/OpenRA.Mods.Common/Pathfinder/PathSearch.cs @@ -11,6 +11,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Runtime.CompilerServices; using OpenRA.Mods.Common.Traits; using OpenRA.Primitives; @@ -18,6 +19,14 @@ namespace OpenRA.Mods.Common.Pathfinder { public sealed class PathSearch : BasePathSearch { + static readonly ConditionalWeakTable LayerPoolTable = new ConditionalWeakTable(); + static readonly ConditionalWeakTable.CreateValueCallback CreateLayerPool = world => new CellInfoLayerPool(world.Map); + + static CellInfoLayerPool LayerPoolForWorld(World world) + { + return LayerPoolTable.GetValue(world, CreateLayerPool); + } + public override IEnumerable> Considered { get { return considered; } @@ -35,7 +44,7 @@ namespace OpenRA.Mods.Common.Pathfinder public static IPathSearch Search(World world, MobileInfo mi, Actor self, bool checkForBlocked, Func goalCondition) { - var graph = new PathGraph(CellInfoLayerManager.Instance.NewLayer(world.Map), mi, self, world, checkForBlocked); + var graph = new PathGraph(LayerPoolForWorld(world), mi, self, world, checkForBlocked); var search = new PathSearch(graph); search.isGoal = goalCondition; search.heuristic = loc => 0; @@ -44,7 +53,7 @@ namespace OpenRA.Mods.Common.Pathfinder public static IPathSearch FromPoint(World world, MobileInfo mi, Actor self, CPos from, CPos target, bool checkForBlocked) { - var graph = new PathGraph(CellInfoLayerManager.Instance.NewLayer(world.Map), mi, self, world, checkForBlocked); + var graph = new PathGraph(LayerPoolForWorld(world), mi, self, world, checkForBlocked); var search = new PathSearch(graph) { heuristic = DefaultEstimator(target) @@ -64,7 +73,7 @@ namespace OpenRA.Mods.Common.Pathfinder public static IPathSearch FromPoints(World world, MobileInfo mi, Actor self, IEnumerable froms, CPos target, bool checkForBlocked) { - var graph = new PathGraph(CellInfoLayerManager.Instance.NewLayer(world.Map), mi, self, world, checkForBlocked); + var graph = new PathGraph(LayerPoolForWorld(world), mi, self, world, checkForBlocked); var search = new PathSearch(graph) { heuristic = DefaultEstimator(target) diff --git a/OpenRA.Mods.Common/Traits/Harvester.cs b/OpenRA.Mods.Common/Traits/Harvester.cs index 589c268708..83f0f7efb8 100644 --- a/OpenRA.Mods.Common/Traits/Harvester.cs +++ b/OpenRA.Mods.Common/Traits/Harvester.cs @@ -165,23 +165,24 @@ namespace OpenRA.Mods.Common.Traits select new { Location = r.Actor.Location + r.Trait.DeliveryOffset, Actor = r.Actor, Occupancy = linkedHarvs }).ToDictionary(r => r.Location); // Start a search from each refinery's delivery location: + List path; var mi = self.Info.Traits.Get(); - var path = self.World.WorldActor.Trait().FindPath( - PathSearch.FromPoints(self.World, mi, self, refs.Values.Select(r => r.Location), self.Location, false) - .WithCustomCost(loc => - { - if (!refs.ContainsKey(loc)) - return 0; + using (var search = PathSearch.FromPoints(self.World, mi, self, refs.Values.Select(r => r.Location), self.Location, false) + .WithCustomCost(loc => + { + if (!refs.ContainsKey(loc)) + return 0; - var occupancy = refs[loc].Occupancy; + var occupancy = refs[loc].Occupancy; - // 4 harvesters clogs up the refinery's delivery location: - if (occupancy >= 3) - return Constants.InvalidNode; + // 4 harvesters clogs up the refinery's delivery location: + if (occupancy >= 3) + return Constants.InvalidNode; - // Prefer refineries with less occupancy (multiplier is to offset distance cost): - return occupancy * 12; - })); + // Prefer refineries with less occupancy (multiplier is to offset distance cost): + return occupancy * 12; + })) + path = self.World.WorldActor.Trait().FindPath(search); if (path.Count != 0) return refs[path.Last()].Actor; diff --git a/OpenRA.Mods.Common/Traits/World/PathFinder.cs b/OpenRA.Mods.Common/Traits/World/PathFinder.cs index 405f2b2a28..55dae83a6d 100644 --- a/OpenRA.Mods.Common/Traits/World/PathFinder.cs +++ b/OpenRA.Mods.Common/Traits/World/PathFinder.cs @@ -72,9 +72,10 @@ namespace OpenRA.Mods.Common.Traits return EmptyPath; } - var pb = FindBidiPath( - PathSearch.FromPoint(world, mi, self, target, source, true), - PathSearch.FromPoint(world, mi, self, source, target, true).Reverse()); + List pb; + using (var fromSrc = PathSearch.FromPoint(world, mi, self, target, source, true)) + using (var fromDest = PathSearch.FromPoint(world, mi, self, source, target, true).Reverse()) + pb = FindBidiPath(fromSrc, fromDest); CheckSanePath2(pb, source, target); @@ -106,11 +107,9 @@ namespace OpenRA.Mods.Common.Traits return EmptyPath; } - var path = FindBidiPath( - PathSearch.FromPoints(world, mi, self, tilesInRange, source, true), - PathSearch.FromPoint(world, mi, self, source, targetCell, true).Reverse()); - - return path; + using (var fromSrc = PathSearch.FromPoints(world, mi, self, tilesInRange, source, true)) + using (var fromDest = PathSearch.FromPoint(world, mi, self, source, targetCell, true).Reverse()) + return FindBidiPath(fromSrc, fromDest); } public List FindPath(IPathSearch search)