From b4882a8b03d8dbe99469b278f3633d9678231506 Mon Sep 17 00:00:00 2001 From: RoosterDragon Date: Thu, 22 Aug 2024 19:52:16 +0100 Subject: [PATCH] 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. --- OpenRA.Game/Exts.cs | 34 +++++++++++++++++++++++----------- OpenRA.Game/MiniYaml.cs | 23 +++++++++++++++++++---- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/OpenRA.Game/Exts.cs b/OpenRA.Game/Exts.cs index 8b8f577b87..2a9ae19272 100644 --- a/OpenRA.Game/Exts.cs +++ b/OpenRA.Game/Exts.cs @@ -14,6 +14,7 @@ using System.Collections.Generic; 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 @@ namespace OpenRA public static Dictionary ToDictionaryWithConflictLog( this IEnumerable source, Func keySelector, Func elementSelector, string debugName, Func logKey = null, Func logValue = null) + { + var output = new Dictionary(); + IntoDictionaryWithConflictLog(source, keySelector, elementSelector, debugName, output, logKey, logValue); + return output; + } + + public static void IntoDictionaryWithConflictLog( + this IEnumerable source, Func keySelector, Func elementSelector, + string debugName, Dictionary output, + Func logKey = null, Func 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>(); + Dictionary> dupKeys = null; var capacity = source is ICollection collection ? collection.Count : 0; - var d = new Dictionary(capacity); + output.Clear(); + output.EnsureCapacity(capacity); foreach (var item in source) { var key = keySelector(item); @@ -451,14 +463,15 @@ namespace OpenRA continue; // Check for a key conflict: - if (!d.TryAdd(key, element)) + if (!output.TryAdd(key, element)) { + dupKeys ??= new Dictionary>(); if (!dupKeys.TryGetValue(key, out var dupKeyMessages)) { // Log the initial conflicting value already inserted: dupKeyMessages = new List { - logValue(d[key]) + logValue(output[key]) }; dupKeys.Add(key, dupKeyMessages); } @@ -469,15 +482,14 @@ namespace OpenRA } // 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) diff --git a/OpenRA.Game/MiniYaml.cs b/OpenRA.Game/MiniYaml.cs index 63613fef36..2a75a65104 100644 --- a/OpenRA.Game/MiniYaml.cs +++ b/OpenRA.Game/MiniYaml.cs @@ -115,6 +115,7 @@ namespace OpenRA const int SpacesPerLevel = 4; static readonly Func StringIdentity = s => s; static readonly Func MiniYamlIdentity = my => my; + static readonly Dictionary ConflictScratch = new(); public readonly string Value; public readonly ImmutableArray Nodes; @@ -419,7 +420,8 @@ namespace OpenRA // 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.Empty); + var result = ResolveInherits(nodes, tree, ImmutableDictionary.Empty); + return result as List ?? result.ToList(); } static void MergeIntoResolved(MiniYamlNode overrideNode, List existingNodes, HashSet existingNodeKeys, @@ -444,9 +446,12 @@ namespace OpenRA existingNodes.Add(overrideNode.WithValue(value)); } - static List ResolveInherits( + static IReadOnlyCollection ResolveInherits( MiniYaml node, Dictionary tree, ImmutableDictionary inherited) { + if (node.Nodes.Length == 0) + return node.Nodes; + var resolved = new List(node.Nodes.Length); var resolvedKeys = new HashSet(node.Nodes.Length); @@ -491,6 +496,9 @@ namespace OpenRA /// static IReadOnlyCollection MergeSelfPartial(IReadOnlyCollection existingNodes) { + if (existingNodes.Count == 0) + return existingNodes; + var keys = new HashSet(existingNodes.Count); var ret = new List(existingNodes.Count); foreach (var n in existingNodes) @@ -511,8 +519,15 @@ namespace OpenRA 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;