Merge pull request #10208 from RoosterDragon/perf-comments

Added some performance comments

OpenRA.Game/Actor.cs

@@ -97,6 +97,9 @@ namespace OpenRA
 				}
 			}
 
+			// PERF: Cache all these traits as soon as the actor is created. This is a fairly cheap one-off cost per
+			// actor that allows us to provide some fast implementations of commonly used methods that are relied on by
+			// performance-sensitive parts of the core game engine, such as pathfinding, visibility and rendering.
 			Bounds = DetermineBounds();
 			VisualBounds = DetermineVisualBounds();
 			EffectiveOwner = TraitOrDefault<IEffectiveOwner>();
@@ -149,6 +152,7 @@ namespace OpenRA
 
 		public IEnumerable<IRenderable> Render(WorldRenderer wr)
 		{
+			// PERF: Avoid LINQ.
 			var renderables = Renderables(wr);
 			foreach (var modifier in renderModifiers)
 				renderables = modifier.ModifyRender(this, wr, renderables);
@@ -157,6 +161,13 @@ namespace OpenRA
 
 		IEnumerable<IRenderable> Renderables(WorldRenderer wr)
 		{
+			// PERF: Avoid LINQ.
+			// Implementations of Render are permitted to return both an eagerly materialized collection or a lazily
+			// generated sequence.
+			// For large amounts of renderables, a lazily generated sequence (e.g. as returned by LINQ, or by using
+			// `yield`) will avoid the need to allocate a large collection.
+			// For small amounts of renderables, allocating a small collection can often be faster and require less
+			// memory than creating the objects needed to represent a sequence.
 			foreach (var render in renders)
 				foreach (var renderable in render.Render(this, wr))
 					yield return renderable;
@@ -206,6 +217,7 @@ namespace OpenRA
 
 		public override string ToString()
 		{
+			// PERF: Avoid format strings.
 			var name = Info.Name + " " + ActorID;
 			if (!IsInWorld)
 				name += " (not in world)";
@@ -305,6 +317,7 @@ namespace OpenRA
 
 		public bool IsDisabled()
 		{
+			// PERF: Avoid LINQ.
 			foreach (var disable in disables)
 				if (disable.Disabled)
 					return true;
@@ -313,6 +326,7 @@ namespace OpenRA
 
 		public bool CanBeViewedByPlayer(Player player)
 		{
+			// PERF: Avoid LINQ.
 			foreach (var visibilityModifier in visibilityModifiers)
 				if (!visibilityModifier.IsVisible(this, player))
 					return false;

OpenRA.Game/Exts.cs

@@ -113,6 +113,7 @@ namespace OpenRA
 
 		public static bool HasModifier(this Modifiers k, Modifiers mod)
 		{
+			// PERF: Enum.HasFlag is slower and requires allocations.
 			return (k & mod) == mod;
 		}
 

OpenRA.Game/Graphics/SpriteFont.cs

@@ -40,6 +40,7 @@ namespace OpenRA.Graphics
 
 			glyphs = new Cache<Pair<char, Color>, GlyphInfo>(CreateGlyph, Pair<char, Color>.EqualityComparer);
 
+			// PERF: Cache these delegates for Measure calls.
 			Func<char, float> characterWidth = character => glyphs[Pair.New(character, Color.White)].Advance;
 			lineWidth = line => line.Sum(characterWidth);
 

OpenRA.Game/Graphics/Viewport.cs

@@ -22,6 +22,7 @@ namespace OpenRA.Graphics
 	{
 		public static bool Includes(this ScrollDirection d, ScrollDirection s)
 		{
+			// PERF: Enum.HasFlag is slower and requires allocations.
 			return (d & s) == s;
 		}
 

OpenRA.Game/Map/Map.cs

@@ -1023,7 +1023,7 @@ namespace OpenRA
 			var uv = cell.ToMPos(this);
 			var terrainIndex = cachedTerrainIndexes[uv];
 
-			// Cache terrain indexes per cell on demand.
+			// PERF: Cache terrain indexes per cell on demand.
 			if (terrainIndex == InvalidCachedTerrainIndex)
 			{
 				var custom = CustomTerrain[uv];

OpenRA.Game/Network/SyncReport.cs

@@ -196,13 +196,20 @@ namespace OpenRA.Network
 
 			static Func<ISync, object> SerializableCopyOfMember(MemberExpression getMember, Type memberType, string name)
 			{
+				// We need to serialize a copy of the current value so if the sync report is generated, the values can
+				// be dumped as strings.
 				if (memberType.IsValueType)
 				{
-					// We can get a copy just by accessing the member since it is a value type.
+					// PERF: For value types we can avoid the overhead of calling ToString immediately. We can instead
+					// just box a copy of the current value into an object. This is faster than calling ToString. We
+					// can call ToString later when we generate the report. Most of the time, the sync report is never
+					// generated so we successfully avoid the overhead to calling ToString.
 					var boxedCopy = Expression.Convert(getMember, typeof(object));
 					return Expression.Lambda<Func<ISync, object>>(boxedCopy, name, new[] { syncParam }).Compile();
 				}
 
+				// For reference types, we have to call ToString right away to get a snapshot of the value. We cannot
+				// delay, as calling ToString later may produce different results.
 				return MemberToString(getMember, memberType, name);
 			}
 

OpenRA.Game/Renderer.cs

@@ -113,6 +113,7 @@ namespace OpenRA
 
 		public void SetViewportParams(int2 scroll, float zoom)
 		{
+			// PERF: Calling SetViewportParams on each renderer is slow. Only call it when things change.
 			var resolutionChanged = lastResolution != Resolution;
 			if (resolutionChanged)
 			{

OpenRA.Game/TraitDictionary.cs

@@ -34,21 +34,19 @@ namespace OpenRA
 		}
 	}
 
+	/// <summary>
+	/// Provides efficient ways to query a set of actors by their traits.
+	/// </summary>
 	class TraitDictionary
 	{
-		// construct this delegate once.
-		static Func<Type, ITraitContainer> doCreateTraitContainer = CreateTraitContainer;
-		static ITraitContainer CreateTraitContainer(Type t)
-		{
-			return (ITraitContainer)typeof(TraitContainer<>).MakeGenericType(t)
-				.GetConstructor(Type.EmptyTypes).Invoke(null);
-		}
+		static readonly Func<Type, ITraitContainer> CreateTraitContainer = t =>
+			(ITraitContainer)typeof(TraitContainer<>).MakeGenericType(t).GetConstructor(Type.EmptyTypes).Invoke(null);
 
-		Dictionary<Type, ITraitContainer> traits = new Dictionary<Type, ITraitContainer>();
+		readonly Dictionary<Type, ITraitContainer> traits = new Dictionary<Type, ITraitContainer>();
 
 		ITraitContainer InnerGet(Type t)
 		{
-			return traits.GetOrAdd(t, doCreateTraitContainer);
+			return traits.GetOrAdd(t, CreateTraitContainer);
 		}
 
 		TraitContainer<T> InnerGet<T>()
@@ -166,6 +164,7 @@ namespace OpenRA
 
 			public IEnumerable<T> GetMultiple(uint actor)
 			{
+				// PERF: Custom enumerator for efficiency - using `yield` is slower.
 				++Queries;
 				return new MultipleEnumerable(this, actor);
 			}
@@ -202,6 +201,7 @@ namespace OpenRA
 
 			public IEnumerable<TraitPair<T>> All()
 			{
+				// PERF: Custom enumerator for efficiency - using `yield` is slower.
 				++Queries;
 				return new AllEnumerable(this);
 			}

OpenRA.Game/Traits/Player/FrozenActorLayer.cs

@@ -84,10 +84,9 @@ namespace OpenRA.Traits
 		{
 			var wasVisible = Visible;
 			Shrouded = true;
-
-			// We are doing the following LINQ manually for performance since this is a hot path.
-			// Visible = !Footprint.Any(shroud.IsVisible);
 			Visible = true;
+
+			// PERF: Avoid LINQ.
 			foreach (var puv in Footprint)
 			{
 				if (shroud.IsVisible(puv))
@@ -128,7 +127,7 @@ namespace OpenRA.Traits
 
 		public bool ShouldBeRemoved(Player owner)
 		{
-			// We use a loop here for performance reasons
+			// PERF: Avoid LINQ.
 			foreach (var rfa in removeFrozenActors)
 				if (rfa.RemoveActor(actor, owner))
 					return true;

OpenRA.Game/Traits/TraitsInterfaces.cs

@@ -50,6 +50,7 @@ namespace OpenRA.Traits
 	{
 		public static bool HasStance(this Stance s, Stance stance)
 		{
+			// PERF: Enum.HasFlag is slower and requires allocations.
 			return (s & stance) == stance;
 		}
 	}
@@ -92,7 +93,11 @@ namespace OpenRA.Traits
 
 	public static class TargetModifiersExts
 	{
-		public static bool HasModifier(this TargetModifiers self, TargetModifiers m) { return (self & m) == m; }
+		public static bool HasModifier(this TargetModifiers self, TargetModifiers m)
+		{
+			// PERF: Enum.HasFlag is slower and requires allocations.
+			return (self & m) == m;
+		}
 	}
 
 	public interface IOrderTargeter

OpenRA.Game/Traits/Util.cs

@@ -74,10 +74,15 @@ namespace OpenRA.Traits
 
 		public static Activity RunActivity(Actor self, Activity act)
 		{
+			// PERF: If there are no activities we can bail straight away and save ourselves a call to
+			// Stopwatch.GetTimestamp.
 			if (act == null)
 				return act;
 
-			// Note - manual iteration here for performance due to high call volume.
+			// PERF: This is a hot path and must run with minimal added overhead.
+			// Calling Stopwatch.GetTimestamp is a bit expensive, so we enumerate manually to allow us to call it only
+			// once per iteration in the normal case.
+			// See also: DoTimed
 			var longTickThresholdInStopwatchTicks = PerfTimer.LongTickThresholdInStopwatchTicks;
 			var start = Stopwatch.GetTimestamp();
 			while (act != null)

OpenRA.Game/Traits/World/ActorMap.cs

@@ -183,7 +183,7 @@ namespace OpenRA.Traits
 				for (var col = 0; col < cols; col++)
 					bins[row * cols + col] = new Bin();
 
-			// Cache this delegate so it does not have to be allocated repeatedly.
+			// PERF: Cache this delegate so it does not have to be allocated repeatedly.
 			actorShouldBeRemoved = removeActorPosition.Contains;
 		}
 
@@ -219,6 +219,7 @@ namespace OpenRA.Traits
 
 		public IEnumerable<Actor> GetActorsAt(CPos a)
 		{
+			// PERF: Custom enumerator for efficiency - using `yield` is slower.
 			var uv = a.ToMPos(map);
 			if (!influence.Contains(uv))
 				return Enumerable.Empty<Actor>();
@@ -534,6 +535,7 @@ namespace OpenRA.Traits
 
 		public IEnumerable<Actor> ActorsInBox(WPos a, WPos b)
 		{
+			// PERF: Inline BinsInBox here to avoid allocations as this method is called often.
 			var left = Math.Min(a.X, b.X);
 			var top = Math.Min(a.Y, b.Y);
 			var right = Math.Max(a.X, b.X);
@@ -559,10 +561,5 @@ namespace OpenRA.Traits
 				}
 			}
 		}
-
-		public IEnumerable<Actor> ActorsInWorld()
-		{
-			return bins.SelectMany(bin => bin.Actors.Where(actor => actor.IsInWorld));
-		}
 	}
 }

OpenRA.Game/Widgets/Widget.cs

@@ -261,6 +261,7 @@ namespace OpenRA.Widgets
 
 		public virtual Rectangle GetEventBounds()
 		{
+			// PERF: Avoid LINQ.
 			var bounds = EventBounds;
 			foreach (var child in Children)
 				if (child.IsVisible())

OpenRA.Game/Widgets/WidgetUtils.cs

@@ -79,7 +79,12 @@ namespace OpenRA.Widgets
 			return new[] { (int)ss[0].Size.Y, (int)ss[1].Size.Y, (int)ss[2].Size.X, (int)ss[3].Size.X };
 		}
 
-		static bool HasFlags(this PanelSides a, PanelSides b) { return (a & b) == b; }
+		static bool HasFlags(this PanelSides a, PanelSides b)
+		{
+			// PERF: Enum.HasFlag is slower and requires allocations.
+			return (a & b) == b;
+		}
+
 		public static Rectangle InflateBy(this Rectangle rect, int l, int t, int r, int b)
 		{
 			return Rectangle.FromLTRB(rect.Left - l, rect.Top - t,

OpenRA.Game/Widgets/WorldInteractionControllerWidget.cs

@@ -304,7 +304,7 @@ namespace OpenRA.Widgets
 
 		static IEnumerable<Actor> SelectActorsInWorld(World world, IEnumerable<string> selectionClasses, Player player)
 		{
-			return SelectActorsByOwnerAndSelectionClass(world.ActorMap.ActorsInWorld(), player, selectionClasses);
+			return SelectActorsByOwnerAndSelectionClass(world.Actors.Where(a => a.IsInWorld), player, selectionClasses);
 		}
 
 		static IEnumerable<Actor> SelectActorsByOwnerAndSelectionClass(IEnumerable<Actor> actors, Player owner, IEnumerable<string> selectionClasses)

OpenRA.Game/WorldUtils.cs

@@ -43,7 +43,10 @@ namespace OpenRA
 
 		public static void DoTimed<T>(this IEnumerable<T> e, Action<T> a, string text)
 		{
-			// Note - manual enumeration here for performance due to high call volume.
+			// PERF: This is a hot path and must run with minimal added overhead.
+			// Calling Stopwatch.GetTimestamp is a bit expensive, so we enumerate manually to allow us to call it only
+			// once per iteration in the normal case.
+			// See also: RunActivity
 			var longTickThresholdInStopwatchTicks = PerfTimer.LongTickThresholdInStopwatchTicks;
 			using (var enumerator = e.GetEnumerator())
 			{

OpenRA.Mods.Common/Effects/Contrail.cs

@@ -61,7 +61,7 @@ namespace OpenRA.Mods.Common.Effects
 
 		public IEnumerable<IRenderable> Render(Actor self, WorldRenderer wr)
 		{
-			yield return trail;
+			return new IRenderable[] { trail };
 		}
 	}
 }

OpenRA.Mods.Common/Pathfinder/PathSearch.cs

@@ -19,6 +19,8 @@ namespace OpenRA.Mods.Common.Pathfinder
 {
 	public sealed class PathSearch : BasePathSearch
 	{
+		// PERF: Maintain a pool of layers used for paths searches for each world. These searches are performed often
+		// so we wish to avoid the high cost of initializing a new search space every time by reusing the old ones.
 		static readonly ConditionalWeakTable<World, CellInfoLayerPool> LayerPoolTable = new ConditionalWeakTable<World, CellInfoLayerPool>();
 		static readonly ConditionalWeakTable<World, CellInfoLayerPool>.CreateValueCallback CreateLayerPool = world => new CellInfoLayerPool(world.Map);
 

OpenRA.Mods.Common/Scripting/Properties/PlayerProperties.cs

@@ -78,7 +78,7 @@ namespace OpenRA.Mods.Common.Scripting
 			if (!Context.World.Map.Rules.Actors.TryGetValue(type, out ai))
 				throw new LuaException("Unknown actor type '{0}'".F(type));
 
-			result.AddRange(Player.World.ActorMap.ActorsInWorld()
+			result.AddRange(Player.World.Actors
 				.Where(actor => actor.Owner == Player && !actor.IsDead && actor.IsInWorld && actor.Info.Name == ai.Name));
 
 			return result.ToArray();

OpenRA.Mods.Common/ShroudExts.cs

@@ -20,6 +20,7 @@ namespace OpenRA.Mods.Common
 	{
 		public static bool AnyExplored(this Shroud shroud, OccupiedCells cells)
 		{
+			// PERF: Avoid LINQ.
 			foreach (var cell in cells)
 				if (shroud.IsExplored(cell.First))
 					return true;
@@ -29,6 +30,7 @@ namespace OpenRA.Mods.Common
 
 		public static bool AnyVisible(this Shroud shroud, OccupiedCells cells)
 		{
+			// PERF: Avoid LINQ.
 			foreach (var cell in cells)
 				if (shroud.IsVisible(cell.First))
 					return true;

OpenRA.Mods.Common/Traits/Mobile.cs

@@ -33,6 +33,7 @@ namespace OpenRA.Mods.Common.Traits
 	{
 		public static bool HasCellCondition(this CellConditions c, CellConditions cellCondition)
 		{
+			// PERF: Enum.HasFlag is slower and requires allocations.
 			return (c & cellCondition) == cellCondition;
 		}
 	}
@@ -132,6 +133,8 @@ namespace OpenRA.Mods.Common.Traits
 			internal readonly TerrainInfo[] TerrainInfos;
 			internal WorldMovementInfo(World world, MobileInfo info)
 			{
+				// PERF: This struct allows us to cache the terrain info for the tileset used by the world.
+				// This allows us to speed up some performance-sensitive pathfinding calculations.
 				World = world;
 				TerrainInfos = info.TilesetTerrainInfo[world.TileSet];
 			}
@@ -217,6 +220,7 @@ namespace OpenRA.Mods.Common.Traits
 			if (SharesCell && world.ActorMap.HasFreeSubCell(cell))
 				return true;
 
+			// PERF: Avoid LINQ.
 			foreach (var otherActor in world.ActorMap.GetActorsAt(cell))
 				if (IsBlockedBy(self, otherActor, ignoreActor, check))
 					return false;
@@ -242,6 +246,7 @@ namespace OpenRA.Mods.Common.Traits
 				return true;
 
 			// If the other actor in our way cannot be crushed, we are blocked.
+			// PERF: Avoid LINQ.
 			var crushables = otherActor.TraitsImplementing<ICrushable>();
 			var lacksCrushability = true;
 			foreach (var crushable in crushables)

OpenRA.Mods.Common/Traits/Modifiers/FrozenUnderFog.cs

@@ -115,6 +115,7 @@ namespace OpenRA.Mods.Common.Traits
 				}
 				else
 				{
+					// PERF: Minimize lookup cost by combining all state into one, and using an array rather than a dictionary.
 					var state = stateByPlayerIndex[i];
 					frozenActor = state.FrozenActor;
 					isVisible = !frozenActor.Visible;

OpenRA.Mods.Common/Traits/Player/ClassicProductionQueue.cs

@@ -48,6 +48,7 @@ namespace OpenRA.Mods.Common.Traits
 
 		public override void Tick(Actor self)
 		{
+			// PERF: Avoid LINQ.
 			isActive = false;
 			foreach (var x in self.World.ActorsWithTrait<Production>())
 			{

OpenRA.Mods.Common/Traits/Render/RenderNameTag.cs

@@ -51,7 +51,7 @@ namespace OpenRA.Mods.Common.Traits
 			var spaceBuffer = (int)(10 / wr.Viewport.Zoom);
 			var effectPos = wr.ProjectedPosition(new int2(pos.X, bounds.Y - spaceBuffer));
 
-			yield return new TextRenderable(font, effectPos, 0, color, name);
+			return new IRenderable[] { new TextRenderable(font, effectPos, 0, color, name) };
 		}
 	}
 }

OpenRA.Mods.Common/Traits/Render/RenderVoxels.cs

@@ -103,10 +103,10 @@ namespace OpenRA.Mods.Common.Traits
 				initializePalettes = false;
 			}
 
-			yield return new VoxelRenderable(
+			return new IRenderable[] { new VoxelRenderable(
 				components, self.CenterPosition, 0, camera, info.Scale,
 				lightSource, info.LightAmbientColor, info.LightDiffuseColor,
-				colorPalette, normalsPalette, shadowPalette);
+				colorPalette, normalsPalette, shadowPalette) };
 		}
 
 		public string Image { get { return info.Image ?? self.Info.Name; } }

OpenRA.Mods.Common/Traits/Render/WithParachute.cs

@@ -134,20 +134,20 @@ namespace OpenRA.Mods.Common.Traits
 		public IEnumerable<IRenderable> Render(Actor self, WorldRenderer wr)
 		{
 			if (info.ShadowImage == null)
-				yield break;
+				return Enumerable.Empty<IRenderable>();
 
 			if (IsTraitDisabled)
-				yield break;
+				return Enumerable.Empty<IRenderable>();
 
 			if (self.IsDead || !self.IsInWorld)
-				yield break;
+				return Enumerable.Empty<IRenderable>();
 
 			if (self.World.FogObscures(self))
-				yield break;
+				return Enumerable.Empty<IRenderable>();
 
 			shadow.Tick();
 			var pos = self.CenterPosition - new WVec(0, 0, self.CenterPosition.Z);
-			yield return new SpriteRenderable(shadow.Image, pos, info.ShadowOffset, info.ShadowZOffset, wr.Palette(info.ShadowPalette), 1, true);
+			return new IRenderable[] { new SpriteRenderable(shadow.Image, pos, info.ShadowOffset, info.ShadowZOffset, wr.Palette(info.ShadowPalette), 1, true) };
 		}
 	}
 }

OpenRA.Mods.Common/Traits/World/ShroudRenderer.cs

@@ -226,6 +226,8 @@ namespace OpenRA.Mods.Common.Traits
 
 		void DirtyCells(IEnumerable<PPos> cells)
 		{
+			// PERF: Many cells in the shroud change every tick. We only track the changes here and defer the real work
+			// we need to do until we render. This allows us to avoid wasted work.
 			cellsDirty.UnionWith(cells);
 		}
 

OpenRA.Mods.Common/Widgets/RadarWidget.cs

@@ -208,6 +208,8 @@ namespace OpenRA.Mods.Common.Widgets
 
 		void MarkShroudDirty(IEnumerable<PPos> projectedCellsChanged)
 		{
+			// PERF: Many cells in the shroud change every tick. We only track the changes here and defer the real work
+			// we need to do until we render. This allows us to avoid wasted work.
 			dirtyShroudCells.UnionWith(projectedCellsChanged);
 		}