perf(templateengine): memoise CollisionDetector's composed-module walk per check (review 2026-10-21 04/P8b)

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:

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<ResolvedMember>();
+        var moduleMemo = new Dictionary<int, IReadOnlyList<RelativeMember>>();
 
         // 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<int>());
+                    moduleMemo);
             }
         }
 
         // Collect inherited members (walk parent chain)
-        CollectInheritedMembers(template, lookup, allMembers, new HashSet<int> { template.Id });
+        CollectInheritedMembers(template, lookup, allMembers, moduleMemo, new HashSet<int> { template.Id });
 
         // Detect duplicates by canonical name
         var collisions = new List<string>();
@@ -83,9 +84,9 @@ public static class CollisionDetector
     /// <see cref="CollectInheritedMembers"/>. 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 <c>false</c> 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, <see cref="GetModuleMembers"/>, 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.)
     /// </param>
     private static void CollectDirectMembers(
         Template template,
@@ -116,43 +117,115 @@ public static class CollisionDetector
         }
     }
 
+    /// <summary>
+    /// A composed module's member, named relative to the module: <see cref="RelativeName"/>
+    /// is the member's path below the slot, <see cref="ModulePath"/> the nested-slot path
+    /// of the module that declares it (<c>null</c> for the module's own direct members).
+    /// </summary>
+    private sealed record RelativeMember(string RelativeName, string MemberType, string? ModulePath);
+
+    /// <summary>
+    /// Adds a composed module's members — its own and its nested modules', recursively —
+    /// under <paramref name="prefix"/>.
+    ///
+    /// <para>
+    /// The module's member list is computed once per collision check and memoised by
+    /// template id in <paramref name="moduleMemo"/> (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.
+    /// </para>
+    /// </summary>
     private static void CollectComposedMembers(
         Template template,
         string prefix,
         Dictionary<int, Template> lookup,
         List<ResolvedMember> members,
-        HashSet<int> visited)
+        Dictionary<int, IReadOnlyList<RelativeMember>> 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<int>(), 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}'"));
+        }
+    }
 
+    /// <summary>
+    /// Computes (or returns the memoised) module-relative member list of
+    /// <paramref name="template"/> as a composed module.
+    /// </summary>
+    /// <param name="template">The composed module.</param>
+    /// <param name="lookup">All templates by id.</param>
+    /// <param name="moduleMemo">Per-check memo of complete (cycle-free) module member lists.</param>
+    /// <param name="activePath">
+    /// 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.
+    /// </param>
+    /// <param name="truncatedByCycle">
+    /// 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.
+    /// </param>
+    /// <returns>The module's members, relative to the module.</returns>
+    private static IReadOnlyList<RelativeMember> GetModuleMembers(
+        Template template,
+        Dictionary<int, Template> lookup,
+        Dictionary<int, IReadOnlyList<RelativeMember>> moduleMemo,
+        HashSet<int> 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<RelativeMember>();
 
-            // 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<int, Template> lookup,
         List<ResolvedMember> members,
+        Dictionary<int, IReadOnlyList<RelativeMember>> moduleMemo,
         HashSet<int> visited)
     {
         if (!template.ParentTemplateId.HasValue)
@@ -186,11 +260,11 @@ public static class CollisionDetector
                     composition.InstanceName,
                     lookup,
                     members,
-                    new HashSet<int>());
+                    moduleMemo);
             }
         }
 
         // Continue up the inheritance chain
-        CollectInheritedMembers(parent, lookup, members, visited);
+        CollectInheritedMembers(parent, lookup, members, moduleMemo, visited);
     }
 }

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
+    // ========================================================================
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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<TemplateComposition>
+        {
+            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<Template> { root, mid, leaf });
+
+        Assert.Equal(1, midCompositions.Enumerations); // pre-fix: 3 (once per slot)
+        var collision = Assert.Single(collisions);
+        Assert.Contains("'c.y.V'", collision);
+        Assert.Contains("module 'c.y'", collision);
+    }
+
+    /// <summary>An <see cref="ICollection{T}"/> that counts how often it is enumerated.</summary>
+    private sealed class CountingCollection<T> : List<T>, ICollection<T>
+    {
+        public int Enumerations { get; set; }
+
+        IEnumerator<T> IEnumerable<T>.GetEnumerator()
+        {
+            Enumerations++;
+            return GetEnumerator();
+        }
+    }
 }