Avoid some allocations in MiniYaml.Merge.

During the merge operation, it is quite common to be dealing with a node that has no child nodes. When there are no such nodes, we can return early from some functions to avoid allocating new collections that will not be used.

In the MergePartial operation, reuse a dictionary as scratch space when checking for conflicts. We introduce a IntoDictionaryWithConflictLog helper to allow this. This avoids allocating a new dictionary for the conflict log that gets thrown away at each check.

Author: RoosterDragon

OpenRA.Game/Exts.cs

@@ -14,6 +14,7 @@
 using System.Globalization;
 using System.Linq;
 using System.Reflection;
+using System.Text;
 using OpenRA.Primitives;
 using OpenRA.Support;
 using OpenRA.Traits;
@@ -432,15 +433,26 @@ public static Dictionary<TKey, TSource> ToDictionaryWithConflictLog<TSource, TKe
 		public static Dictionary<TKey, TElement> ToDictionaryWithConflictLog<TSource, TKey, TElement>(
 			this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, Func<TSource, TElement> elementSelector,
 			string debugName, Func<TKey, string> logKey = null, Func<TElement, string> logValue = null)
+		{
+			var output = new Dictionary<TKey, TElement>();
+			IntoDictionaryWithConflictLog(source, keySelector, elementSelector, debugName, output, logKey, logValue);
+			return output;
+		}
+
+		public static void IntoDictionaryWithConflictLog<TSource, TKey, TElement>(
+			this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, Func<TSource, TElement> elementSelector,
+			string debugName, Dictionary<TKey, TElement> output,
+			Func<TKey, string> logKey = null, Func<TElement, string> logValue = null)
 		{
 			// Fall back on ToString() if null functions are provided:
 			logKey ??= s => s.ToString();
 			logValue ??= s => s.ToString();
 
 			// Try to build a dictionary and log all duplicates found (if any):
-			var dupKeys = new Dictionary<TKey, List<string>>();
+			Dictionary<TKey, List<string>> dupKeys = null;
 			var capacity = source is ICollection<TSource> collection ? collection.Count : 0;
-			var d = new Dictionary<TKey, TElement>(capacity);
+			output.Clear();
+			output.EnsureCapacity(capacity);
 			foreach (var item in source)
 			{
 				var key = keySelector(item);
@@ -451,14 +463,15 @@ public static Dictionary<TKey, TElement> ToDictionaryWithConflictLog<TSource, TK
 					continue;
 
 				// Check for a key conflict:
-				if (!d.TryAdd(key, element))
+				if (!output.TryAdd(key, element))
 				{
+					dupKeys ??= new Dictionary<TKey, List<string>>();
 					if (!dupKeys.TryGetValue(key, out var dupKeyMessages))
 					{
 						// Log the initial conflicting value already inserted:
 						dupKeyMessages = new List<string>
 						{
-							logValue(d[key])
+							logValue(output[key])
 						};
 						dupKeys.Add(key, dupKeyMessages);
 					}
@@ -469,15 +482,14 @@ public static Dictionary<TKey, TElement> ToDictionaryWithConflictLog<TSource, TK
 			}
 
 			// If any duplicates were found, throw a descriptive error
-			if (dupKeys.Count > 0)
+			if (dupKeys != null)
 			{
-				var badKeysFormatted = string.Join(", ", dupKeys.Select(p => $"{logKey(p.Key)}: [{string.Join(",", p.Value)}]"));
-				var msg = $"{debugName}, duplicate values found for the following keys: {badKeysFormatted}";
-				throw new ArgumentException(msg);
+				var badKeysFormatted = new StringBuilder(
+					$"{debugName}, duplicate values found for the following keys: ");
+				foreach (var p in dupKeys)
+					badKeysFormatted.Append($"{logKey(p.Key)}: [{string.Join(",", p.Value)}]");
+				throw new ArgumentException(badKeysFormatted.ToString());
 			}
-
-			// Return the dictionary we built:
-			return d;
 		}
 
 		public static Color ColorLerp(float t, Color c1, Color c2)

OpenRA.Game/MiniYaml.cs

@@ -115,6 +115,7 @@ public sealed class MiniYaml
 		const int SpacesPerLevel = 4;
 		static readonly Func<string, string> StringIdentity = s => s;
 		static readonly Func<MiniYaml, MiniYaml> MiniYamlIdentity = my => my;
+		static readonly Dictionary<string, MiniYamlNode> ConflictScratch = new();
 
 		public readonly string Value;
 		public readonly ImmutableArray<MiniYamlNode> Nodes;
@@ -419,7 +420,8 @@ public static List<MiniYamlNode> Merge(IEnumerable<IReadOnlyCollection<MiniYamlN
 
 			// Resolve any top-level removals (e.g. removing whole actor blocks)
 			var nodes = new MiniYaml("", resolved.Select(kv => new MiniYamlNode(kv.Key, kv.Value)));
-			return ResolveInherits(nodes, tree, ImmutableDictionary<string, MiniYamlNode.SourceLocation>.Empty);
+			var result = ResolveInherits(nodes, tree, ImmutableDictionary<string, MiniYamlNode.SourceLocation>.Empty);
+			return result as List<MiniYamlNode> ?? result.ToList();
 		}
 
 		static void MergeIntoResolved(MiniYamlNode overrideNode, List<MiniYamlNode> existingNodes, HashSet<string> existingNodeKeys,
@@ -444,9 +446,12 @@ static void MergeIntoResolved(MiniYamlNode overrideNode, List<MiniYamlNode> exis
 				existingNodes.Add(overrideNode.WithValue(value));
 		}
 
-		static List<MiniYamlNode> ResolveInherits(
+		static IReadOnlyCollection<MiniYamlNode> ResolveInherits(
 			MiniYaml node, Dictionary<string, MiniYaml> tree, ImmutableDictionary<string, MiniYamlNode.SourceLocation> inherited)
 		{
+			if (node.Nodes.Length == 0)
+				return node.Nodes;
+
 			var resolved = new List<MiniYamlNode>(node.Nodes.Length);
 			var resolvedKeys = new HashSet<string>(node.Nodes.Length);
 
@@ -491,6 +496,9 @@ static List<MiniYamlNode> ResolveInherits(
 		/// </summary>
 		static IReadOnlyCollection<MiniYamlNode> MergeSelfPartial(IReadOnlyCollection<MiniYamlNode> existingNodes)
 		{
+			if (existingNodes.Count == 0)
+				return existingNodes;
+
 			var keys = new HashSet<string>(existingNodes.Count);
 			var ret = new List<MiniYamlNode>(existingNodes.Count);
 			foreach (var n in existingNodes)
@@ -511,8 +519,15 @@ static IReadOnlyCollection<MiniYamlNode> MergeSelfPartial(IReadOnlyCollection<Mi
 
 		static MiniYaml MergePartial(MiniYaml existingNodes, MiniYaml overrideNodes)
 		{
-			existingNodes?.Nodes.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => $"{x.Key} (at {x.Location})");
-			overrideNodes?.Nodes.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => $"{x.Key} (at {x.Location})");
+			lock (ConflictScratch)
+			{
+				// PERF: Reuse ConflictScratch for all conflict checks to avoid allocations.
+				existingNodes?.Nodes.IntoDictionaryWithConflictLog(
+					n => n.Key, n => n, "MiniYaml.Merge", ConflictScratch, k => k, n => $"{n.Key} (at {n.Location})");
+				overrideNodes?.Nodes.IntoDictionaryWithConflictLog(
+					n => n.Key, n => n, "MiniYaml.Merge", ConflictScratch, k => k, n => $"{n.Key} (at {n.Location})");
+				ConflictScratch.Clear();
+			}
 
 			if (existingNodes == null)
 				return overrideNodes;