From 0d4f7d33b9f417b27af765e70428008d3de06bf6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:30:27 -0400 Subject: [PATCH] perf(templateengine): memoise CollisionDetector's composed-module walk per check (review 2026-10-21 04/P8b) --- docs/requirements/Component-TemplateEngine.md | 2 + .../CollisionDetector.cs | 126 ++++++++++++++---- .../CollisionDetectorTests.cs | 51 +++++++ 3 files changed, 153 insertions(+), 26 deletions(-) diff --git a/docs/requirements/Component-TemplateEngine.md b/docs/requirements/Component-TemplateEngine.md index 85a7529d..7536cf76 100644 --- a/docs/requirements/Component-TemplateEngine.md +++ b/docs/requirements/Component-TemplateEngine.md @@ -143,6 +143,8 @@ If any composed module introduces a name that already exists (from another compo A derived template stores `IsInherited` placeholder rows that mirror the members it inherits from its parent chain. These placeholders are **excluded** from collision detection on the template's own member set and on the inherited-parent walk — the inheritance walk already re-adds those members under the parent's origin, so counting the placeholder copies as well would report a spurious self-collision against the very member they mirror. (Placeholders are only counted when a composed module is itself a derived template, because the composed-module walk does not climb that module's parent chain and the placeholders are then the sole representation of its inherited members.) Without this exclusion there is no supported way to add an attribute, alarm, script, or composition to a derived template — every add is blocked by false collisions on the pre-existing inherited rows. +A template composed into several slots — or reached through several parents (a diamond) — is checked under **every** slot, since each slot yields distinct canonical names. Its module-relative member list is computed once per collision check and memoised by template id; each further slot only re-prefixes it (review 2026-10-21 04/P8(b)). A genuine composition cycle still contributes nothing at the re-entered module, and a list cut short by a cycle is never memoised. + ## Flattening Process When an instance is deployed, the Template Engine resolves the full configuration: diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs index e4a145e1..6e0eb8df 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs @@ -32,6 +32,7 @@ public static class CollisionDetector // not-yet-saved templates carrying Id 0). var lookup = CycleDetector.BuildLookup(allTemplates); var allMembers = new List(); + var moduleMemo = new Dictionary>(); // Collect direct (top-level) members. Inherited placeholder rows are skipped: // the inheritance walk below re-adds them under the parent's origin. @@ -47,12 +48,12 @@ public static class CollisionDetector prefix: composition.InstanceName, lookup, allMembers, - visited: new HashSet()); + moduleMemo); } } // Collect inherited members (walk parent chain) - CollectInheritedMembers(template, lookup, allMembers, new HashSet { template.Id }); + CollectInheritedMembers(template, lookup, allMembers, moduleMemo, new HashSet { template.Id }); // Detect duplicates by canonical name var collisions = new List(); @@ -83,9 +84,9 @@ public static class CollisionDetector /// . Counting both yielded two distinct /// origins for the same canonical name and a spurious collision, which blocked /// every attempt to add an attribute/composition to a derived template. - /// Pass false only for the composed-module walk, where - /// placeholder rows are the sole representation of a derived module's inherited - /// members (that walk does not climb the composed template's parent chain). + /// (The composed-module walk, , keeps placeholder rows + /// instead: there they are the sole representation of a derived module's inherited + /// members, because that walk does not climb the composed template's parent chain.) /// private static void CollectDirectMembers( Template template, @@ -116,43 +117,115 @@ public static class CollisionDetector } } + /// + /// A composed module's member, named relative to the module: + /// is the member's path below the slot, the nested-slot path + /// of the module that declares it (null for the module's own direct members). + /// + private sealed record RelativeMember(string RelativeName, string MemberType, string? ModulePath); + + /// + /// Adds a composed module's members — its own and its nested modules', recursively — + /// under . + /// + /// + /// The module's member list is computed once per collision check and memoised by + /// template id in (review 2026-10-21 04/P8(b)): a + /// module composed into several slots, or reached through several parents (a diamond), + /// used to have its whole subtree re-walked once per path. Each further slot now only + /// re-prefixes the memoised list. Every path still contributes its own canonical names + /// — a repeatedly-composed template must be checked under every slot. + /// + /// private static void CollectComposedMembers( Template template, string prefix, Dictionary lookup, List members, - HashSet visited) + Dictionary> moduleMemo) { - // `visited` is the current RECURSION PATH, not a global visited-set: an id is - // added on entry and REMOVED on exit (finally), so a template legitimately - // composed into more than one slot is re-descended for each slot. The early - // return fires only on a genuine cycle — an id already on the ACTIVE path. - // (A global visited-set would blind the detector to every collision reachable - // only through the second-and-later slot of a repeatedly-composed template.) - if (!visited.Add(template.Id)) - return; + var relative = GetModuleMembers(template, lookup, moduleMemo, new HashSet(), out _); + foreach (var m in relative) + { + var origin = m.ModulePath == null ? prefix : $"{prefix}.{m.ModulePath}"; + members.Add(new ResolvedMember($"{prefix}.{m.RelativeName}", m.MemberType, $"module '{origin}'")); + } + } + /// + /// Computes (or returns the memoised) module-relative member list of + /// as a composed module. + /// + /// The composed module. + /// All templates by id. + /// Per-check memo of complete (cycle-free) module member lists. + /// + /// The ids on the current recursion PATH (not a global visited-set): a nested module + /// already on the path is a genuine cycle and contributes nothing, exactly as before. + /// + /// + /// True when a cycle cut this list short. Such a list depends on the path it was + /// computed from, so it — and every ancestor list built from it — is not memoised. + /// + /// The module's members, relative to the module. + private static IReadOnlyList GetModuleMembers( + Template template, + Dictionary lookup, + Dictionary> moduleMemo, + HashSet activePath, + out bool truncatedByCycle) + { + truncatedByCycle = false; + if (moduleMemo.TryGetValue(template.Id, out var cached)) + return cached; + + activePath.Add(template.Id); try { - // Add direct members of this composed template with the prefix. Inherited - // placeholder rows are KEPT here: the composed-module walk does not climb the - // composed template's parent chain, so the placeholders are the only - // representation of a derived module's inherited members. - CollectDirectMembers(template, prefix, $"module '{prefix}'", members, skipInherited: false); + var result = new List(); - // Recurse into nested compositions + // Direct members of the composed template. Inherited placeholder rows are + // KEPT here: the composed-module walk does not climb the composed template's + // parent chain, so the placeholders are the only representation of a derived + // module's inherited members. + foreach (var attr in template.Attributes) + result.Add(new RelativeMember(attr.Name, "Attribute", null)); + foreach (var alarm in template.Alarms) + result.Add(new RelativeMember(alarm.Name, "Alarm", null)); + foreach (var script in template.Scripts) + result.Add(new RelativeMember(script.Name, "Script", null)); + + // Nested compositions. foreach (var composition in template.Compositions) { - if (lookup.TryGetValue(composition.ComposedTemplateId, out var nested)) + if (!lookup.TryGetValue(composition.ComposedTemplateId, out var nested)) + continue; + + if (activePath.Contains(nested.Id)) { - var nestedPrefix = $"{prefix}.{composition.InstanceName}"; - CollectComposedMembers(nested, nestedPrefix, lookup, members, visited); + truncatedByCycle = true; + continue; + } + + var slot = composition.InstanceName; + var nestedMembers = GetModuleMembers(nested, lookup, moduleMemo, activePath, out var nestedTruncated); + truncatedByCycle |= nestedTruncated; + foreach (var m in nestedMembers) + { + result.Add(new RelativeMember( + $"{slot}.{m.RelativeName}", + m.MemberType, + m.ModulePath == null ? slot : $"{slot}.{m.ModulePath}")); } } + + if (!truncatedByCycle) + moduleMemo[template.Id] = result; + return result; } finally { - visited.Remove(template.Id); + activePath.Remove(template.Id); } } @@ -160,6 +233,7 @@ public static class CollisionDetector Template template, Dictionary lookup, List members, + Dictionary> moduleMemo, HashSet visited) { if (!template.ParentTemplateId.HasValue) @@ -186,11 +260,11 @@ public static class CollisionDetector composition.InstanceName, lookup, members, - new HashSet()); + moduleMemo); } } // Continue up the inheritance chain - CollectInheritedMembers(parent, lookup, members, visited); + CollectInheritedMembers(parent, lookup, members, moduleMemo, visited); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs index 5fd97d28..2a6d92fa 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs @@ -258,4 +258,55 @@ public class CollisionDetectorTests var collisions = CollisionDetector.DetectCollisions(a, all); Assert.Empty(collisions); } + + // ======================================================================== + // Review 2026-10-21 04/P8(b): a module reached through several slots is walked once + // ======================================================================== + + /// + /// Verifies that a module composed into several slots (a diamond) has its subtree + /// walked once per collision check — every further slot reuses the memoised member + /// list under its own prefix — while still producing every canonical name. + /// + [Fact] + public void DetectCollisions_DiamondComposition_WalksSharedModuleOnce() + { + var leaf = new Template("Leaf") { Id = 3 }; + leaf.Attributes.Add(new TemplateAttribute("V") { Id = 30, TemplateId = 3, DataType = DataType.Float }); + + var mid = new Template("Mid") { Id = 2 }; + var midCompositions = new CountingCollection + { + new TemplateComposition("x") { Id = 20, TemplateId = 2, ComposedTemplateId = 3 }, + new TemplateComposition("y") { Id = 21, TemplateId = 2, ComposedTemplateId = 3 }, + }; + mid.Compositions = midCompositions; + + var root = new Template("Root") { Id = 1 }; + root.Compositions.Add(new TemplateComposition("a") { Id = 10, TemplateId = 1, ComposedTemplateId = 2 }); + root.Compositions.Add(new TemplateComposition("b") { Id = 11, TemplateId = 1, ComposedTemplateId = 2 }); + root.Compositions.Add(new TemplateComposition("c") { Id = 12, TemplateId = 1, ComposedTemplateId = 2 }); + // A genuine collision reachable only through the third slot of the shared module. + root.Attributes.Add(new TemplateAttribute("c.y.V") { Id = 40, TemplateId = 1, DataType = DataType.Float }); + + midCompositions.Enumerations = 0; + var collisions = CollisionDetector.DetectCollisions(root, new List