From bb61d3afc7d1b95abb13c3167cc7e86f0e6d908c Mon Sep 17 00:00:00 2001 From: "Clarence \"Sparr\" Risher" Date: Thu, 29 Oct 2026 17:55:11 -0400 Subject: [PATCH 1/3] Speed up patch node matching Matching each patch against every node spent most of its time allocating, and the allocations stalled the patching thread in Boehm GC waits. In a profiled RP-1/RO repatch, NodeMatcher.IsMatch was 55% of the patching thread, WildcardMatch 36%, and GC waits 22%. - WildcardMatch compares strings directly when the wildcard has no * or ?, keeping the regex's match before a final newline, and caches its regex by the wildcard instead of building the pattern on every call. - CheckConstraints parses each constraint string once and caches it. - CheckConstraints visits the matching subnodes in one pass instead of calling FindConfigNodeIn with each index in turn. - FindConfigNodeIn no longer builds a list of the matching subnodes. - KeyValueCache.FetchWithKey takes a delegate that need not capture. Co-Authored-By: Claude Opus 5.5 --- ModuleManager/Collections/KeyValueCache.cs | 20 ++ ModuleManager/MMPatchLoader.cs | 245 +++++++++++++-------- 2 files changed, 169 insertions(+), 96 deletions(-) diff --git a/ModuleManager/Collections/KeyValueCache.cs b/ModuleManager/Collections/KeyValueCache.cs index 0aaee61b..e59f6627 100644 --- a/ModuleManager/Collections/KeyValueCache.cs +++ b/ModuleManager/Collections/KeyValueCache.cs @@ -25,5 +25,25 @@ public TValue Fetch(TKey key, Func createValue) } } } + + // Takes the key as an argument so that callers can pass a delegate that captures nothing, + // which the compiler caches, instead of allocating a closure on every call. + public TValue FetchWithKey(TKey key, Func createValue) + { + if (createValue == null) throw new ArgumentNullException(nameof(createValue)); + lock(lockObject) + { + if (dict.TryGetValue(key, out TValue value)) + { + return value; + } + else + { + TValue newValue = createValue(key); + dict.Add(key, newValue); + return newValue; + } + } + } } } diff --git a/ModuleManager/MMPatchLoader.cs b/ModuleManager/MMPatchLoader.cs index 5278758b..ef9dc869 100644 --- a/ModuleManager/MMPatchLoader.cs +++ b/ModuleManager/MMPatchLoader.cs @@ -1535,101 +1535,129 @@ public static List SplitConstraints(string condition) static readonly char[] contraintSeparators = { '[', ']' }; - public static bool CheckConstraints(ConfigNode node, string constraints) + // CheckConstraints is called for every node a patch might apply to, with the same few + // constraint strings, so each string is parsed once. + private sealed class ParsedConstraints + { + public bool empty; + public List list; + public char op; + public string type; + public string name; + public string remaining; + } + + private static readonly KeyValueCache constraintsCache = new KeyValueCache(); + private static readonly Func parseConstraints = ParseConstraints; + + private static ParsedConstraints ParseConstraints(string constraints) { + ParsedConstraints parsed = new ParsedConstraints(); constraints = constraints.RemoveWS(); if (constraints.Length == 0) - return true; + { + parsed.empty = true; + return parsed; + } List constraintList = SplitConstraints(constraints); - if (constraintList.Count == 1) + if (constraintList.Count != 1) { - constraints = constraintList[0]; + parsed.list = constraintList; + return parsed; + } - string remainingConstraints = ""; - if (constraints.Contains(":HAS[", out int hasStart)) - { - hasStart += 5; - remainingConstraints = constraints.Substring(hasStart, constraintList[0].LastIndexOf(']') - hasStart); - constraints = constraints.Substring(0, hasStart - 5); - } + constraints = constraintList[0]; + + string remainingConstraints = ""; + if (constraints.Contains(":HAS[", out int hasStart)) + { + hasStart += 5; + remainingConstraints = constraints.Substring(hasStart, constraintList[0].LastIndexOf(']') - hasStart); + constraints = constraints.Substring(0, hasStart - 5); + } - string[] splits = constraints.Split(contraintSeparators, 3); - string type = splits[0].Substring(1); - string name = splits.Length > 1 ? splits[1] : null; + string[] splits = constraints.Split(contraintSeparators, 3); + parsed.type = splits[0].Substring(1); + parsed.name = splits.Length > 1 ? splits[1] : null; + parsed.op = constraints[0]; + parsed.remaining = remainingConstraints; + return parsed; + } - switch (constraints[0]) + public static bool CheckConstraints(ConfigNode node, string constraints) + { + if (constraints == null) throw new ArgumentNullException("input"); + ParsedConstraints parsed = constraintsCache.FetchWithKey(constraints, parseConstraints); + + if (parsed.empty) + return true; + + if (parsed.list != null) + { + bool ret3 = true; + foreach (string constraint in parsed.list) { - case '@': - case '!': + ret3 = ret3 && CheckConstraints(node, constraint); + } + return ret3; + } - // @MODULE[ModuleAlternator] or !MODULE[ModuleAlternator] - bool not = (constraints[0] == '!'); + string type = parsed.type; + string name = parsed.name; + string remainingConstraints = parsed.remaining; - bool any = false; - int index = 0; - ConfigNode last = null; - while (true) - { - ConfigNode subNode = FindConfigNodeIn(node, type, name, index++); - if (subNode == last || subNode == null) - break; - any = any || CheckConstraints(subNode, remainingConstraints); - last = subNode; - } - if (last != null) - { - //print("CheckConstraints: " + constraints + " " + (not ^ any)); - return not ^ any; - } - //print("CheckConstraints: " + constraints + " " + (not ^ false)); - return not ^ false; + switch (parsed.op) + { + case '@': + case '!': - case '#': + // @MODULE[ModuleAlternator] or !MODULE[ModuleAlternator] + bool not = (parsed.op == '!'); - // #module[Winglet] - if (node.HasValue(type) && WildcardMatchValues(node, type, name)) - { - bool ret2 = CheckConstraints(node, remainingConstraints); - //print("CheckConstraints: " + constraints + " " + ret2); - return ret2; - } - //print("CheckConstraints: " + constraints + " false"); - return false; + // Each subnode FindConfigNodeIn (node, type, name, index) returns for index = 0, 1, ... + // until it repeats one, in a single pass. + bool any = false; + ConfigNode last = null; + int c = node.nodes.Count; + for (int i = 0; i < c && !any; ++i) + { + ConfigNode subNode = node.nodes[i]; + if (!WildcardMatch(subNode.name, type)) + continue; + if (name != null && !(subNode.GetValue("name") is string subName && WildcardMatch(subName, name))) + continue; + if (subNode == last) + break; + any = CheckConstraints(subNode, remainingConstraints); + last = subNode; + } + if (last != null) + return not ^ any; + return not ^ false; - case '~': + case '#': - // ~breakingForce[] breakingForce is not present - // or: ~breakingForce[100] will be true if it's present but not 100, too. - if (name == "" && node.HasValue(type)) - { - //print("CheckConstraints: " + constraints + " false"); - return false; - } - if (name != "" && WildcardMatchValues(node, type, name)) - { - //print("CheckConstraints: " + constraints + " false"); - return false; - } - bool ret = CheckConstraints(node, remainingConstraints); - //print("CheckConstraints: " + constraints + " " + ret); - return ret; + // #module[Winglet] + if (node.HasValue(type) && WildcardMatchValues(node, type, name)) + return CheckConstraints(node, remainingConstraints); + return false; - default: - //print("CheckConstraints: " + constraints + " false"); + case '~': + + // ~breakingForce[] breakingForce is not present + // or: ~breakingForce[100] will be true if it's present but not 100, too. + if (name == "" && node.HasValue(type)) return false; - } - } + if (name != "" && WildcardMatchValues(node, type, name)) + return false; + return CheckConstraints(node, remainingConstraints); - bool ret3 = true; - foreach (string constraint in constraintList) - { - ret3 = ret3 && CheckConstraints(node, constraint); + default: + return false; } - //print("CheckConstraints: " + constraints + " " + ret3); - return ret3; } public static bool WildcardMatchValues(ConfigNode node, string type, string value) @@ -1653,16 +1681,23 @@ public static bool WildcardMatchValues(ConfigNode node, string type, string valu return false; } + private static readonly KeyValueCache wildcardRegexCache = new KeyValueCache(); + private static readonly char[] wildcardChars = { '*', '?' }; + public static bool WildcardMatch(string s, string wildcard) { if (wildcard == null) return true; - string pattern = "^" + Regex.Escape(wildcard).Replace(@"\*", ".*").Replace(@"\?", ".") + "$"; + if (s == null) + throw new ArgumentNullException("input"); - Regex regex = regexCache.Fetch(pattern, delegate - { - return new Regex(pattern); - }); + // Without wildcards the pattern is ^literal$, and $ also matches before a final newline. + if (wildcard.IndexOfAny(wildcardChars) < 0) + return s == wildcard + || (s.Length == wildcard.Length + 1 && s[wildcard.Length] == '\n' && string.CompareOrdinal(s, 0, wildcard, 0, wildcard.Length) == 0); + + Regex regex = wildcardRegexCache.FetchWithKey(wildcard, w => + new Regex("^" + Regex.Escape(w).Replace(@"\*", ".*").Replace(@"\?", ".") + "$")); return regex.IsMatch(s); } @@ -1715,41 +1750,59 @@ public static ConfigNode FindConfigNodeIn( string nodeName = null, int index = 0) { - List nodes = new List(); + // Among the subnodes whose type matches nodeType: without nodeName, the index-th (from + // the end if negative, clamped to the first or last); with nodeName, the index-th of + // those whose name matches, or the last one checked if there are fewer than that. int c = src.nodes.Count; - for(int i = 0; i < c; ++i) - { - if (WildcardMatch(src.nodes[i].name, nodeType)) - nodes.Add(src.nodes[i]); - } - int nodeCount = nodes.Count; - if (nodeCount == 0) - return null; + ConfigNode last = null; if (nodeName == null) { if (index >= 0) - return nodes[Math.Min(index, nodeCount - 1)]; - return nodes[Math.Max(0, nodeCount + index)]; + { + for (int i = 0; i < c; ++i) + { + ConfigNode n = src.nodes[i]; + if (WildcardMatch(n.name, nodeType)) + { + last = n; + if (index-- == 0) + return n; + } + } + return last; + } + for (int i = c - 1; i >= 0; --i) + { + ConfigNode n = src.nodes[i]; + if (WildcardMatch(n.name, nodeType)) + { + last = n; + if (++index == 0) + return n; + } + } + return last; } - ConfigNode last = null; if (index >= 0) { - for (int i = 0; i < nodeCount; ++i) + for (int i = 0; i < c; ++i) { - if (nodes[i].HasValue("name") && WildcardMatch(nodes[i].GetValue("name"), nodeName)) + ConfigNode n = src.nodes[i]; + if (WildcardMatch(n.name, nodeType) && n.GetValue("name") is string name && WildcardMatch(name, nodeName)) { - last = nodes[i]; + last = n; if (--index < 0) return last; } } return last; } - for (int i = nodeCount - 1; i >= 0; --i) + for (int i = c - 1; i >= 0; --i) { - if (nodes[i].HasValue("name") && WildcardMatch(nodes[i].GetValue("name"), nodeName)) + ConfigNode n = src.nodes[i]; + if (WildcardMatch(n.name, nodeType) && n.GetValue("name") is string name && WildcardMatch(name, nodeName)) { - last = nodes[i]; + last = n; if (++index >= 0) return last; } From 904fc4bfdc879f979fd4ec41d45ab8451acb721d Mon Sep 17 00:00:00 2001 From: "Clarence \"Sparr\" Risher" Date: Thu, 29 Oct 2026 18:59:10 -0400 Subject: [PATCH 2/3] Index root nodes by type for edit patches Every edit patch tested every root node, and NodeMatcher.IsMatch rejects most of them by type alone. PatchApplier now keeps a PatchTypeIndex of the database's list nodes, grouped by node type in list order, and gives it to edit patches, which visit only the nodes of their matcher's type. Edits replace values in place and cannot rename a root node, so they leave the index valid; it is invalidated if a type changes anyway. Copy and delete patches mark their matcher's type stale and insert patches their node type, and a stale group is rebuilt when next needed. IPatch.Apply is unchanged. Co-Authored-By: Claude Opus 5.5 --- ModuleManager/ModuleManager.csproj | 1 + ModuleManager/NodeMatcher.cs | 3 + ModuleManager/PatchApplier.cs | 26 ++++- ModuleManager/PatchTypeIndex.cs | 69 ++++++++++++ ModuleManager/Patches/EditPatch.cs | 65 ++++++++--- ModuleManagerTests/ModuleManagerTests.csproj | 1 + ModuleManagerTests/PatchApplierTest.cs | 53 +++++++++ ModuleManagerTests/PatchTypeIndexTest.cs | 108 +++++++++++++++++++ 8 files changed, 306 insertions(+), 20 deletions(-) create mode 100644 ModuleManager/PatchTypeIndex.cs create mode 100644 ModuleManagerTests/PatchTypeIndexTest.cs diff --git a/ModuleManager/ModuleManager.csproj b/ModuleManager/ModuleManager.csproj index 20fe0da1..8cbeee25 100644 --- a/ModuleManager/ModuleManager.csproj +++ b/ModuleManager/ModuleManager.csproj @@ -79,6 +79,7 @@ + diff --git a/ModuleManager/NodeMatcher.cs b/ModuleManager/NodeMatcher.cs index 8777de97..d8a92741 100644 --- a/ModuleManager/NodeMatcher.cs +++ b/ModuleManager/NodeMatcher.cs @@ -14,6 +14,9 @@ public class NodeMatcher : INodeMatcher private readonly string[] namePatterns = null; private readonly string constraints = ""; + // Only nodes of this type can match: ConfigNode.name is the type (e.g. PART), not the name value. + public string Type => type; + public NodeMatcher(string type, string name, string constraints) { if (type == string.Empty) throw new ArgumentException("can't be empty", nameof(type)); diff --git a/ModuleManager/PatchApplier.cs b/ModuleManager/PatchApplier.cs index adaade88..fb23687f 100644 --- a/ModuleManager/PatchApplier.cs +++ b/ModuleManager/PatchApplier.cs @@ -23,16 +23,17 @@ public IEnumerable ApplyPatches(IEnumerable patches) if (patches == null) throw new ArgumentNullException(nameof(patches)); LinkedList databaseConfigs = new LinkedList(); + PatchTypeIndex index = new PatchTypeIndex(databaseConfigs); foreach (IPass pass in patches) { - ApplyPatches(databaseConfigs, pass); + ApplyPatches(databaseConfigs, index, pass); } return databaseConfigs; } - private void ApplyPatches(LinkedList databaseConfigs, IPass pass) + private void ApplyPatches(LinkedList databaseConfigs, PatchTypeIndex index, IPass pass) { progress.PassStarted(pass); @@ -40,7 +41,26 @@ private void ApplyPatches(LinkedList databaseConfigs, IPass pas { try { - patch.Apply(databaseConfigs, progress, logger); + // Edit patches, most of them, need only the nodes of one type. Other patches + // add or remove nodes, after which the index rebuilds the groups they can + // affect when next needed: copies and deletes only touch their matcher's + // type, and inserts add one node of their type. + if (patch is EditPatch editPatch) + { + editPatch.Apply(databaseConfigs, index, progress, logger); + } + else + { + if (patch is CopyPatch copyPatch && copyPatch.NodeMatcher is NodeMatcher copyMatcher) + index.Invalidate(copyMatcher.Type); + else if (patch is DeletePatch deletePatch && deletePatch.NodeMatcher is NodeMatcher deleteMatcher) + index.Invalidate(deleteMatcher.Type); + else if (patch is InsertPatch insertPatch) + index.Invalidate(insertPatch.NodeType); + else + index.Invalidate(); + patch.Apply(databaseConfigs, progress, logger); + } if (patch.CountsAsPatch) progress.PatchApplied(); } catch (Exception e) diff --git a/ModuleManager/PatchTypeIndex.cs b/ModuleManager/PatchTypeIndex.cs new file mode 100644 index 00000000..8d7473b5 --- /dev/null +++ b/ModuleManager/PatchTypeIndex.cs @@ -0,0 +1,69 @@ +using System; +using System.Collections.Generic; + +namespace ModuleManager +{ + // The nodes of a config list grouped by node type, each group in list order. Built on first + // use; whoever adds or removes nodes from the list must call Invalidate, with the type of the + // nodes if they are all of one type, so that only that group is rebuilt. + public class PatchTypeIndex + { + private static readonly List> none = new List>(); + + private readonly LinkedList configs; + private Dictionary>> byType; + private readonly HashSet staleTypes = new HashSet(); + + public PatchTypeIndex(LinkedList configs) + { + this.configs = configs ?? throw new ArgumentNullException(nameof(configs)); + } + + public List> NodesOfType(string type) + { + if (type == null) throw new ArgumentNullException(nameof(type)); + if (byType == null) Build(); + else if (staleTypes.Remove(type)) Rebuild(type); + return byType.TryGetValue(type, out List> nodes) ? nodes : none; + } + + public void Invalidate() + { + byType = null; + staleTypes.Clear(); + } + + public void Invalidate(string type) + { + if (type == null) throw new ArgumentNullException(nameof(type)); + if (byType != null) staleTypes.Add(type); + } + + private void Rebuild(string type) + { + List> nodes = new List>(); + for (LinkedListNode listNode = configs.First; listNode != null; listNode = listNode.Next) + { + if (listNode.Value.Node.name == type) nodes.Add(listNode); + } + if (nodes.Count > 0) byType[type] = nodes; + else byType.Remove(type); + } + + private void Build() + { + byType = new Dictionary>>(); + for (LinkedListNode listNode = configs.First; listNode != null; listNode = listNode.Next) + { + string type = listNode.Value.Node.name; + if (type == null) continue; + if (!byType.TryGetValue(type, out List> nodes)) + { + nodes = new List>(); + byType.Add(type, nodes); + } + nodes.Add(listNode); + } + } + } +} diff --git a/ModuleManager/Patches/EditPatch.cs b/ModuleManager/Patches/EditPatch.cs index 91613879..8d6931be 100644 --- a/ModuleManager/Patches/EditPatch.cs +++ b/ModuleManager/Patches/EditPatch.cs @@ -35,24 +35,55 @@ public void Apply(LinkedList databaseConfigs, IPatchProgress pr PatchContext context = new PatchContext(UrlConfig, databaseConfigs, logger, progress); for (LinkedListNode listNode = databaseConfigs.First; listNode != null; listNode = listNode.Next) { - IProtoUrlConfig protoConfig = listNode.Value; - try - { - if (!NodeMatcher.IsMatch(protoConfig.Node)) continue; - if (loop) logger.Info($"Looping on {UrlConfig.SafeUrl()} to {protoConfig.FullUrl}"); - - do - { - progress.ApplyingUpdate(protoConfig, UrlConfig); - listNode.Value = protoConfig = new ProtoUrlConfig(protoConfig.UrlFile, MMPatchLoader.ModifyNode(new NodeStack(protoConfig.Node), UrlConfig.config, context)); - } while (loop && NodeMatcher.IsMatch(protoConfig.Node)); - - if (loop) protoConfig.Node.RemoveNodes("MM_PATCH_LOOP"); - } - catch (Exception ex) + ApplyTo(listNode, context, progress, logger); + } + } + + // Same as Apply, visiting only the nodes the index lists under the matcher's type, which + // are the only ones it can match. An edit replaces each node's value in place and keeps + // its type, so the index stays valid; if a type does change, the index is invalidated. + public void Apply(LinkedList databaseConfigs, PatchTypeIndex index, IPatchProgress progress, IBasicLogger logger) + { + if (index == null) throw new ArgumentNullException(nameof(index)); + if (!(NodeMatcher is NodeMatcher nodeMatcher)) + { + Apply(databaseConfigs, progress, logger); + return; + } + if (databaseConfigs == null) throw new ArgumentNullException(nameof(databaseConfigs)); + if (progress == null) throw new ArgumentNullException(nameof(progress)); + if (logger == null) throw new ArgumentNullException(nameof(logger)); + + string type = nodeMatcher.Type; + bool typeChanged = false; + PatchContext context = new PatchContext(UrlConfig, databaseConfigs, logger, progress); + foreach (LinkedListNode listNode in index.NodesOfType(type)) + { + ApplyTo(listNode, context, progress, logger); + if (listNode.Value.Node.name != type) typeChanged = true; + } + if (typeChanged) index.Invalidate(); + } + + private void ApplyTo(LinkedListNode listNode, PatchContext context, IPatchProgress progress, IBasicLogger logger) + { + IProtoUrlConfig protoConfig = listNode.Value; + try + { + if (!NodeMatcher.IsMatch(protoConfig.Node)) return; + if (loop) logger.Info($"Looping on {UrlConfig.SafeUrl()} to {protoConfig.FullUrl}"); + + do { - progress.Exception(UrlConfig, $"Exception while applying update {UrlConfig.SafeUrl()} to {protoConfig.FullUrl}", ex); - } + progress.ApplyingUpdate(protoConfig, UrlConfig); + listNode.Value = protoConfig = new ProtoUrlConfig(protoConfig.UrlFile, MMPatchLoader.ModifyNode(new NodeStack(protoConfig.Node), UrlConfig.config, context)); + } while (loop && NodeMatcher.IsMatch(protoConfig.Node)); + + if (loop) protoConfig.Node.RemoveNodes("MM_PATCH_LOOP"); + } + catch (Exception ex) + { + progress.Exception(UrlConfig, $"Exception while applying update {UrlConfig.SafeUrl()} to {protoConfig.FullUrl}", ex); } } } diff --git a/ModuleManagerTests/ModuleManagerTests.csproj b/ModuleManagerTests/ModuleManagerTests.csproj index f3afbbf5..a174f3ab 100644 --- a/ModuleManagerTests/ModuleManagerTests.csproj +++ b/ModuleManagerTests/ModuleManagerTests.csproj @@ -108,6 +108,7 @@ + diff --git a/ModuleManagerTests/PatchApplierTest.cs b/ModuleManagerTests/PatchApplierTest.cs index d54ac286..cb111fc5 100644 --- a/ModuleManagerTests/PatchApplierTest.cs +++ b/ModuleManagerTests/PatchApplierTest.cs @@ -1,12 +1,15 @@ using System; using System.Collections.Generic; +using System.Linq; using Xunit; using NSubstitute; using UnityEngine; +using TestUtils; using ModuleManager; using ModuleManager.Collections; using ModuleManager.Logging; using ModuleManager.Patches; +using ModuleManager.Patches.PassSpecifiers; using ModuleManager.Progress; namespace ModuleManagerTests @@ -115,5 +118,55 @@ public void TestApplyPatches() progress.PatchApplied(); }); } + + [Fact] + public void TestApplyPatches__TypeIndex() + { + // Edit patches visit nodes through a per-type index; copies, deletes, and inserts + // between them must be seen by the edits that follow. + IBasicLogger logger = Substitute.For(); + IPatchProgress progress = Substitute.For(); + PatchApplier patchApplier = new PatchApplier(progress, logger); + IPassSpecifier passSpecifier = Substitute.For(); + + IPatch Insert(string type, string name) => new InsertPatch( + UrlBuilder.CreateConfig("abc/" + name, new TestConfigNode(type) { { "name", name } }), type, passSpecifier); + UrlDir.UrlConfig Config(string url, ConfigNode node) => UrlBuilder.CreateConfig(url, node); + + IPass pass1 = Substitute.For(); + pass1.GetEnumerator().Returns(new ArrayEnumerator( + Insert("PART", "a"), + Insert("PROP", "p"), + Insert("PART", "b") + )); + + IPass pass2 = Substitute.For(); + pass2.GetEnumerator().Returns(new ArrayEnumerator( + new EditPatch(Config("ghi/e1", new TestConfigNode("@PART[*]") { { "x", "1" } }), new NodeMatcher("PART", "*", null), passSpecifier), + new CopyPatch(Config("ghi/c1", new TestConfigNode("+PART[a]") { { "@name", "c" } }), new NodeMatcher("PART", "a", null), passSpecifier), + new EditPatch(Config("ghi/e2", new TestConfigNode("@PART[*]") { { "y", "2" } }), new NodeMatcher("PART", "*", null), passSpecifier), + new DeletePatch(Config("ghi/d1", new ConfigNode("!PART[b]")), new NodeMatcher("PART", "b", null), passSpecifier), + new EditPatch(Config("ghi/e3", new TestConfigNode("@PART[*]") { { "z", "3" } }), new NodeMatcher("PART", "*", null), passSpecifier), + Insert("PART", "d"), + new EditPatch(Config("ghi/e4", new TestConfigNode("@PART[d]") { { "w", "4" } }), new NodeMatcher("PART", "d", null), passSpecifier) + )); + + IProtoUrlConfig[] configs = patchApplier.ApplyPatches(new[] { pass1, pass2 }).ToArray(); + + progress.DidNotReceiveWithAnyArgs().Error(null, null); + progress.DidNotReceiveWithAnyArgs().Exception(null, null); + progress.DidNotReceiveWithAnyArgs().Exception(null, null, null); + + Assert.Equal(new[] { "PART", "PART", "PROP", "PART" }, configs.Select(c => c.Node.name)); + AssertNodesEqual(new TestConfigNode("PART") { { "name", "a" }, { "x", "1" }, { "y", "2" }, { "z", "3" } }, configs[0].Node); + AssertNodesEqual(new TestConfigNode("PART") { { "name", "c" }, { "x", "1" }, { "y", "2" }, { "z", "3" } }, configs[1].Node); + AssertNodesEqual(new TestConfigNode("PROP") { { "name", "p" } }, configs[2].Node); + AssertNodesEqual(new TestConfigNode("PART") { { "name", "d" }, { "w", "4" } }, configs[3].Node); + } + + private void AssertNodesEqual(ConfigNode expected, ConfigNode actual) + { + Assert.Equal(expected.ToString(), actual.ToString()); + } } } diff --git a/ModuleManagerTests/PatchTypeIndexTest.cs b/ModuleManagerTests/PatchTypeIndexTest.cs new file mode 100644 index 00000000..2c714028 --- /dev/null +++ b/ModuleManagerTests/PatchTypeIndexTest.cs @@ -0,0 +1,108 @@ +using System; +using System.Collections.Generic; +using Xunit; +using TestUtils; +using ModuleManager; + +namespace ModuleManagerTests +{ + public class PatchTypeIndexTest + { + private readonly UrlDir.UrlFile file = UrlBuilder.CreateFile("abc/def.cfg"); + + [Fact] + public void TestConstructor__ConfigsNull() + { + ArgumentNullException ex = Assert.Throws(delegate + { + new PatchTypeIndex(null); + }); + + Assert.Equal("configs", ex.ParamName); + } + + [Fact] + public void TestNodesOfType__TypeNull() + { + PatchTypeIndex index = new PatchTypeIndex(new LinkedList()); + ArgumentNullException ex = Assert.Throws(delegate + { + index.NodesOfType(null); + }); + + Assert.Equal("type", ex.ParamName); + } + + [Fact] + public void TestNodesOfType() + { + LinkedList configs = new LinkedList(); + LinkedListNode a1 = configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("A"))); + LinkedListNode b1 = configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("B"))); + LinkedListNode a2 = configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("A"))); + configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("a"))); + + PatchTypeIndex index = new PatchTypeIndex(configs); + + Assert.Equal(new[] { a1, a2 }, index.NodesOfType("A")); + Assert.Equal(new[] { b1 }, index.NodesOfType("B")); + Assert.Empty(index.NodesOfType("C")); + } + + [Fact] + public void TestInvalidate() + { + LinkedList configs = new LinkedList(); + LinkedListNode a1 = configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("A"))); + LinkedListNode a2 = configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("A"))); + + PatchTypeIndex index = new PatchTypeIndex(configs); + Assert.Equal(new[] { a1, a2 }, index.NodesOfType("A")); + + LinkedListNode a3 = configs.AddAfter(a1, new ProtoUrlConfig(file, new ConfigNode("A"))); + configs.Remove(a2); + + // Not seen until invalidated + Assert.Equal(new[] { a1, a2 }, index.NodesOfType("A")); + + index.Invalidate(); + Assert.Equal(new[] { a1, a3 }, index.NodesOfType("A")); + } + + [Fact] + public void TestInvalidate__Type() + { + LinkedList configs = new LinkedList(); + LinkedListNode a1 = configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("A"))); + LinkedListNode b1 = configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("B"))); + + PatchTypeIndex index = new PatchTypeIndex(configs); + List> bNodes = index.NodesOfType("B"); + + LinkedListNode a2 = configs.AddFirst(new ProtoUrlConfig(file, new ConfigNode("A"))); + LinkedListNode c1 = configs.AddLast(new ProtoUrlConfig(file, new ConfigNode("C"))); + index.Invalidate("A"); + index.Invalidate("C"); + + Assert.Equal(new[] { a2, a1 }, index.NodesOfType("A")); + Assert.Equal(new[] { c1 }, index.NodesOfType("C")); + Assert.Same(bNodes, index.NodesOfType("B")); + + configs.Remove(c1); + index.Invalidate("C"); + Assert.Empty(index.NodesOfType("C")); + } + + [Fact] + public void TestInvalidate__TypeNull() + { + PatchTypeIndex index = new PatchTypeIndex(new LinkedList()); + ArgumentNullException ex = Assert.Throws(delegate + { + index.Invalidate(null); + }); + + Assert.Equal("type", ex.ParamName); + } + } +} From 46e359d90382303d3433d11089d7c6d3c636882c Mon Sep 17 00:00:00 2001 From: "Clarence \"Sparr\" Risher" Date: Fri, 30 Oct 2026 11:06:03 -0400 Subject: [PATCH 3/3] Match patches against each config's stored name value NodeMatcher.IsMatch looked up a node's name value for every patch with a name pattern, once per node of the patch's type. With the previous two commits applied, in a profiled RP-1/RO repatch, that lookup was about 15% of the patching thread. ProtoUrlConfig already looked up the name value for FullUrl; it now keeps the Value object and exposes its value as NameValue. NodeMatcher.IsMatch(IProtoUrlConfig) matches against it, and edit, copy, and delete patches call that through an INodeMatcher extension that falls back to IsMatch(ConfigNode) for other matchers. A special command (*@PART[foo]/name = bar) writes to that Value object in place, so its renames are seen. Edits and copies modify a copy and wrap it in a new ProtoUrlConfig, so their renames are seen too. Co-Authored-By: Claude Opus 5.5 --- ModuleManager/NodeMatcher.cs | 27 +++++- ModuleManager/Patches/CopyPatch.cs | 2 +- ModuleManager/Patches/DeletePatch.cs | 2 +- ModuleManager/Patches/EditPatch.cs | 4 +- ModuleManager/ProtoUrlConfig.cs | 20 ++++- ModuleManagerTests/ModuleManagerTests.csproj | 1 + ModuleManagerTests/NodeMatcherTest.cs | 50 +++++++++++ ModuleManagerTests/PatchApplierRenameTest.cs | 90 ++++++++++++++++++++ ModuleManagerTests/ProtoUrlConfigTest.cs | 23 ++++- 9 files changed, 211 insertions(+), 8 deletions(-) create mode 100644 ModuleManagerTests/PatchApplierRenameTest.cs diff --git a/ModuleManager/NodeMatcher.cs b/ModuleManager/NodeMatcher.cs index d8a92741..05eb4058 100644 --- a/ModuleManager/NodeMatcher.cs +++ b/ModuleManager/NodeMatcher.cs @@ -36,10 +36,24 @@ public NodeMatcher(string type, string name, string constraints) public bool IsMatch(ConfigNode node) { if (node.name != type) return false; + return IsNameAndConstraintsMatch(node, namePatterns != null ? node.GetValue("name") : null); + } + + // Same as IsMatch(protoConfig.Node), but takes the name value a ProtoUrlConfig already read. + public bool IsMatch(IProtoUrlConfig protoConfig) + { + ConfigNode node = protoConfig.Node; + if (node.name != type) return false; + string name = null; + if (namePatterns != null) + name = protoConfig is ProtoUrlConfig known ? known.NameValue : node.GetValue("name"); + return IsNameAndConstraintsMatch(node, name); + } + private bool IsNameAndConstraintsMatch(ConfigNode node, string name) + { if (namePatterns != null) { - string name = node.GetValue("name"); if (name == null) return false; bool match = false; @@ -58,4 +72,15 @@ public bool IsMatch(ConfigNode node) return MMPatchLoader.CheckConstraints(node, constraints); } } + + public static class NodeMatcherExtensions + { + // Uses NodeMatcher.IsMatch(IProtoUrlConfig) when available, so patches can match a config + // without looking up its name again. Other matchers get the node. + public static bool IsMatch(this INodeMatcher nodeMatcher, IProtoUrlConfig protoConfig) + { + if (nodeMatcher is NodeMatcher matcher) return matcher.IsMatch(protoConfig); + return nodeMatcher.IsMatch(protoConfig.Node); + } + } } diff --git a/ModuleManager/Patches/CopyPatch.cs b/ModuleManager/Patches/CopyPatch.cs index 45ee09fe..478f4e9b 100644 --- a/ModuleManager/Patches/CopyPatch.cs +++ b/ModuleManager/Patches/CopyPatch.cs @@ -35,7 +35,7 @@ public void Apply(LinkedList databaseConfigs, IPatchProgress pr IProtoUrlConfig protoConfig = listNode.Value; try { - if (!NodeMatcher.IsMatch(protoConfig.Node)) continue; + if (!NodeMatcher.IsMatch(protoConfig)) continue; ConfigNode clone = MMPatchLoader.ModifyNode(new NodeStack(protoConfig.Node), UrlConfig.config, context); if (protoConfig.Node.GetValue("name") is string name && name == clone.GetValue("name")) diff --git a/ModuleManager/Patches/DeletePatch.cs b/ModuleManager/Patches/DeletePatch.cs index 46d46a94..9161b379 100644 --- a/ModuleManager/Patches/DeletePatch.cs +++ b/ModuleManager/Patches/DeletePatch.cs @@ -34,7 +34,7 @@ public void Apply(LinkedList databaseConfigs, IPatchProgress pr try { LinkedListNode nextNode = currentNode.Next; - if (NodeMatcher.IsMatch(protoConfig.Node)) + if (NodeMatcher.IsMatch(protoConfig)) { progress.ApplyingDelete(protoConfig, UrlConfig); databaseConfigs.Remove(currentNode); diff --git a/ModuleManager/Patches/EditPatch.cs b/ModuleManager/Patches/EditPatch.cs index 8d6931be..dff4d17e 100644 --- a/ModuleManager/Patches/EditPatch.cs +++ b/ModuleManager/Patches/EditPatch.cs @@ -70,14 +70,14 @@ private void ApplyTo(LinkedListNode listNode, PatchContext cont IProtoUrlConfig protoConfig = listNode.Value; try { - if (!NodeMatcher.IsMatch(protoConfig.Node)) return; + if (!NodeMatcher.IsMatch(protoConfig)) return; if (loop) logger.Info($"Looping on {UrlConfig.SafeUrl()} to {protoConfig.FullUrl}"); do { progress.ApplyingUpdate(protoConfig, UrlConfig); listNode.Value = protoConfig = new ProtoUrlConfig(protoConfig.UrlFile, MMPatchLoader.ModifyNode(new NodeStack(protoConfig.Node), UrlConfig.config, context)); - } while (loop && NodeMatcher.IsMatch(protoConfig.Node)); + } while (loop && NodeMatcher.IsMatch(protoConfig)); if (loop) protoConfig.Node.RemoveNodes("MM_PATCH_LOOP"); } diff --git a/ModuleManager/ProtoUrlConfig.cs b/ModuleManager/ProtoUrlConfig.cs index 71f39df3..e4e89aee 100644 --- a/ModuleManager/ProtoUrlConfig.cs +++ b/ModuleManager/ProtoUrlConfig.cs @@ -23,6 +23,13 @@ public class ProtoUrlConfig : IProtoUrlConfig public string NodeType => Node.name; public string FullUrl { get; } + // The node's name value, or null if it had none when this was created, so that NodeMatcher + // need not look it up again for every patch. Holds the Value object because a special + // command (*@PART[foo]/name = bar) writes to it in place. Edit and copy patches can add or + // remove name values, but they modify a copy and wrap it in a new ProtoUrlConfig. + private readonly ConfigNode.Value nameValue; + public string NameValue => nameValue?.value; + public ProtoUrlConfig(UrlDir.UrlFile urlFile, ConfigNode node) { UrlFile = urlFile ?? throw new ArgumentNullException(nameof(urlFile)); @@ -30,8 +37,17 @@ public ProtoUrlConfig(UrlDir.UrlFile urlFile, ConfigNode node) FileUrl = UrlFile.url + '.' + urlFile.fileExtension; FullUrl = FileUrl + '/' + Node.name; - if (node.GetValue("name") is string nameValue) - FullUrl += '[' + nameValue + ']'; + for (int i = 0; i < node.values.Count; ++i) + { + if (node.values[i].name == "name") + { + nameValue = node.values[i]; + break; + } + } + + if (NameValue != null) + FullUrl += '[' + NameValue + ']'; } } } diff --git a/ModuleManagerTests/ModuleManagerTests.csproj b/ModuleManagerTests/ModuleManagerTests.csproj index a174f3ab..4634e059 100644 --- a/ModuleManagerTests/ModuleManagerTests.csproj +++ b/ModuleManagerTests/ModuleManagerTests.csproj @@ -109,6 +109,7 @@ + diff --git a/ModuleManagerTests/NodeMatcherTest.cs b/ModuleManagerTests/NodeMatcherTest.cs index 866c69fc..49e60d67 100644 --- a/ModuleManagerTests/NodeMatcherTest.cs +++ b/ModuleManagerTests/NodeMatcherTest.cs @@ -1,5 +1,6 @@ using System; using Xunit; +using NSubstitute; using TestUtils; using ModuleManager; @@ -329,5 +330,54 @@ public void TestIsMatch__Constraints_Open() } #endregion + + [Fact] + public void TestIsMatch__ProtoUrlConfig() + { + NodeMatcher matcher = new NodeMatcher("NODE", "blah", null); + UrlDir.UrlFile file = UrlBuilder.CreateFile("abc/def.cfg"); + + Assert.True(matcher.IsMatch(new ProtoUrlConfig(file, new TestConfigNode("NODE") { { "name", "blah" } }))); + Assert.False(matcher.IsMatch(new ProtoUrlConfig(file, new TestConfigNode("NODE") { { "name", "bleh" } }))); + Assert.False(matcher.IsMatch(new ProtoUrlConfig(file, new ConfigNode("NODE")))); + Assert.False(matcher.IsMatch(new ProtoUrlConfig(file, new TestConfigNode("PART") { { "name", "blah" } }))); + } + + [Fact] + public void TestIsMatch__ProtoUrlConfig__NameChangedInPlace() + { + NodeMatcher matcher = new NodeMatcher("NODE", "blah", null); + ConfigNode node = new TestConfigNode("NODE") { { "name", "bleh" } }; + ProtoUrlConfig protoConfig = new ProtoUrlConfig(UrlBuilder.CreateFile("abc/def.cfg"), node); + + Assert.False(matcher.IsMatch(protoConfig)); + + node.SetValue("name", "blah"); + + Assert.True(matcher.IsMatch(protoConfig)); + Assert.True(matcher.IsMatch(node)); + } + + [Fact] + public void TestIsMatch__ProtoUrlConfig__Constraints() + { + NodeMatcher matcher = new NodeMatcher("NODE", null, "@INNER"); + UrlDir.UrlFile file = UrlBuilder.CreateFile("abc/def.cfg"); + + Assert.True(matcher.IsMatch(new ProtoUrlConfig(file, new TestConfigNode("NODE") { new ConfigNode("INNER") }))); + Assert.False(matcher.IsMatch(new ProtoUrlConfig(file, new ConfigNode("NODE")))); + } + + [Fact] + public void TestIsMatch__Extension__OtherMatcher() + { + INodeMatcher matcher = NSubstitute.Substitute.For(); + ConfigNode node = new ConfigNode("NODE"); + IProtoUrlConfig protoConfig = NSubstitute.Substitute.For(); + protoConfig.Node.Returns(node); + matcher.IsMatch(node).Returns(true); + + Assert.True(NodeMatcherExtensions.IsMatch(matcher, protoConfig)); + } } } diff --git a/ModuleManagerTests/PatchApplierRenameTest.cs b/ModuleManagerTests/PatchApplierRenameTest.cs new file mode 100644 index 00000000..5471410d --- /dev/null +++ b/ModuleManagerTests/PatchApplierRenameTest.cs @@ -0,0 +1,90 @@ +using System.Linq; +using Xunit; +using NSubstitute; +using TestUtils; +using ModuleManager; +using ModuleManager.Collections; +using ModuleManager.Logging; +using ModuleManager.Patches; +using ModuleManager.Patches.PassSpecifiers; +using ModuleManager.Progress; + +namespace ModuleManagerTests +{ + public class PatchApplierRenameTest + { + [Fact] + public void TestApplyPatches__Rename() + { + // Patches that change a node's name value are matched by its new name afterward, + // through the name value each new ProtoUrlConfig stores. + IPatchProgress progress = Substitute.For(); + PatchApplier patchApplier = new PatchApplier(progress, Substitute.For()); + IPassSpecifier passSpecifier = Substitute.For(); + + IPatch Insert(string name) => new InsertPatch( + UrlBuilder.CreateConfig("abc/" + name, new TestConfigNode("PART") { { "name", name } }), "PART", passSpecifier); + IPatch Edit(string url, string name, string key, string value) => new EditPatch( + UrlBuilder.CreateConfig(url, new TestConfigNode("@PART[" + name + "]") { { key, value } }), new NodeMatcher("PART", name, null), passSpecifier); + + IPass pass1 = Substitute.For(); + pass1.GetEnumerator().Returns(new ArrayEnumerator(Insert("a"), Insert("other"))); + + IPass pass2 = Substitute.For(); + pass2.GetEnumerator().Returns(new ArrayEnumerator( + Edit("ghi/e1", "a", "@name", "b"), + Edit("ghi/e2", "b", "x", "1"), + Edit("ghi/e3", "a", "y", "2"), + new CopyPatch(UrlBuilder.CreateConfig("ghi/c1", new TestConfigNode("+PART[b]") { { "@name", "c" } }), new NodeMatcher("PART", "b", null), passSpecifier), + Edit("ghi/e4", "c", "z", "3"), + Edit("ghi/e5", "b", "w", "4"), + new DeletePatch(UrlBuilder.CreateConfig("ghi/d1", new ConfigNode("!PART[other]")), new NodeMatcher("PART", "other", null), passSpecifier) + )); + + IProtoUrlConfig[] configs = patchApplier.ApplyPatches(new[] { pass1, pass2 }).ToArray(); + + progress.DidNotReceiveWithAnyArgs().Error(null, null); + progress.DidNotReceiveWithAnyArgs().Exception(null, null); + progress.DidNotReceiveWithAnyArgs().Exception(null, null, null); + + Assert.Equal(2, configs.Length); + Assert.Equal(new TestConfigNode("PART") { { "name", "b" }, { "x", "1" }, { "w", "4" } }.ToString(), configs[0].Node.ToString()); + Assert.Equal(new TestConfigNode("PART") { { "name", "c" }, { "x", "1" }, { "z", "3" } }.ToString(), configs[1].Node.ToString()); + } + + [Fact] + public void TestApplyPatches__Rename__SpecialCommand() + { + // A special command writes to another root node's name value in place, with no new + // ProtoUrlConfig, and patches match that node by its new name afterward. + IPatchProgress progress = Substitute.For(); + PatchApplier patchApplier = new PatchApplier(progress, Substitute.For()); + IPassSpecifier passSpecifier = Substitute.For(); + + IPatch Insert(string name) => new InsertPatch( + UrlBuilder.CreateConfig("abc/" + name, new TestConfigNode("PART") { { "name", name } }), "PART", passSpecifier); + IPatch Edit(string url, string name, string key, string value) => new EditPatch( + UrlBuilder.CreateConfig(url, new TestConfigNode("@PART[" + name + "]") { { key, value } }), new NodeMatcher("PART", name, null), passSpecifier); + + IPass pass1 = Substitute.For(); + pass1.GetEnumerator().Returns(new ArrayEnumerator(Insert("source"), Insert("target"))); + + IPass pass2 = Substitute.For(); + pass2.GetEnumerator().Returns(new ArrayEnumerator( + Edit("ghi/e1", "source", "*@PART[target]/name", "renamed"), + Edit("ghi/e2", "renamed", "x", "1"), + Edit("ghi/e3", "target", "y", "2") + )); + + IProtoUrlConfig[] configs = patchApplier.ApplyPatches(new[] { pass1, pass2 }).ToArray(); + + progress.DidNotReceiveWithAnyArgs().Error(null, null); + progress.DidNotReceiveWithAnyArgs().Exception(null, null); + progress.DidNotReceiveWithAnyArgs().Exception(null, null, null); + + Assert.Equal(2, configs.Length); + Assert.Equal(new TestConfigNode("PART") { { "name", "source" } }.ToString(), configs[0].Node.ToString()); + Assert.Equal(new TestConfigNode("PART") { { "name", "renamed" }, { "x", "1" } }.ToString(), configs[1].Node.ToString()); + } + } +} diff --git a/ModuleManagerTests/ProtoUrlConfigTest.cs b/ModuleManagerTests/ProtoUrlConfigTest.cs index f16cd1bd..1b85e882 100644 --- a/ModuleManagerTests/ProtoUrlConfigTest.cs +++ b/ModuleManagerTests/ProtoUrlConfigTest.cs @@ -1,4 +1,4 @@ -using System; +using System; using Xunit; using TestUtils; using ModuleManager; @@ -83,5 +83,26 @@ public void TestFullUrl__NameValue() Assert.Equal("abc/def.cfg/SOME_NODE[some_value]", protoUrlConfig.FullUrl); } + + [Fact] + public void TestNameValue() + { + ConfigNode node = new TestConfigNode("SOME_NODE") + { + { "name", "some_value" }, + }; + + ProtoUrlConfig protoUrlConfig = new ProtoUrlConfig(UrlBuilder.CreateFile("abc/def.cfg"), node); + + Assert.Equal("some_value", protoUrlConfig.NameValue); + } + + [Fact] + public void TestNameValue__None() + { + ProtoUrlConfig protoUrlConfig = new ProtoUrlConfig(UrlBuilder.CreateFile("abc/def.cfg"), new ConfigNode("SOME_NODE")); + + Assert.Null(protoUrlConfig.NameValue); + } } }