From dca822dc59b37ca2f784546d41ae7546108f5587 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 10:17:49 -0400 Subject: [PATCH] perf(opcua): de-quadratic the lock-time teardown paths (B3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review 01 P1/P2/P3/P5 — work done under the manager-wide SDK Lock on the pinned publish thread, where every Read/Browse/Publish waits. - P1 RebuildAddressSpace detaches the shared root's top-level children once instead of every leaf from its parent (NodeState.RemoveChild is a linear scan + RemoveAt, ~k²/2 compares per k-child folder); root notifiers are cleared as a list instead of one O(F) RemoveRootNotifier each; ForgetPredefinedSubtree / TombstoneMonitoredSubtree are iterative with one child buffer. - P2 RemoveEquipmentSubtree walks a parent→children index maintained by EnsureFolder/EnsureVariable/MaterialiseAlarmCondition (keyed by the parent id the caller named) instead of prefix-scanning six maps per removed equipment; a surviving condition's wiring into a removed folder is found through the folder's own notifier list; descendants are not detached from a folder that is itself being discarded. - P3 LowerAllEventsMonitoring(condition) replaces the full O(C) reconcile on RemoveAlarmConditionNode / RemoveEquipmentSubtree / the kind-swap. - P5 AlarmNotifierWiring.Folders is a reference-identity HashSet and an already-wired pair skips the SDK's linear AddNotifier dedup. Benchmark (NodeManagerLockTimeBenchmarkTests, M-series Mac under load, three runs each): rebuild of a 10k-tag folder 75–150 ms → 5–6 ms; removing 200 equipment (250 signals each) from a 50k-node space 471–681 ms → 58–64 ms; all-events bookkeeping visits for 10 condition removals over 50 conditions 900 → 10. The two timing legs assert < 1 s; the P3 leg asserts the visit count (machine-independent). Refs #732 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../OtOpcUaNodeManager.cs | 331 +++++++++++------- .../NodeManagerLockTimeBenchmarkTests.cs | 190 ++++++++++ 2 files changed, 397 insertions(+), 124 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerLockTimeBenchmarkTests.cs diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 774fc3f6..9a2fe15f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -111,6 +111,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// against it (a miss ⇒ BadHistoryOperationUnsupported). Cleared on /// . private readonly ConcurrentDictionary _eventNotifierSources = new(); + /// Review 01 P2 — parent key → the keys of the folders / variables / conditions materialised + /// under it (by the parent id the CALLER named, so a node whose parent was unknown at creation and hangs + /// under the shared root is still found under its intended parent). + /// walks this instead of prefix-scanning every map per removed equipment. Guarded by Lock. + private readonly Dictionary> _childIndex = new(); + /// The reverse of : child key → parent key. Guarded by Lock. + private readonly Dictionary _parentIndex = new(); private FolderState? _root; private readonly ILogger _logger; @@ -1400,8 +1407,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 ForgetPredefinedSubtree(existing); // The materialise tail raises only the new instance's unit (no full reconcile), so give the // dropped instance's back here rather than leave it tracked until the next full reconcile. - if (_allEventsMonitoredConditions.Remove(existing)) - existing.SetAreEventsMonitored(SystemContext, false, includeChildren: true); + LowerAllEventsMonitoring(existing); } // H6a: re-materialising the same id as the OTHER kind (native↔scripted) must reflect the new @@ -1579,6 +1585,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 AddPredefinedNode(SystemContext, alarm); _alarmConditions[conditionKey] = alarm; + if (!string.IsNullOrEmpty(equipmentNodeId)) IndexChild(MapKey(realm, equipmentNodeId), conditionKey); // Only the new condition can be missing its unit here, so raise just that one — a full reconcile // walks every condition, which made a deploy of N alarms O(N²) under Lock whenever any Server-object // item was live (always, in production). Removal, rebuild and (un)subscribe keep the full reconcile. @@ -1690,11 +1697,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } /// One materialised native-alarm condition together with the equipment folders wired as extra - /// event-notifier roots for it. The list is mutated in place under Lock as - /// notifiers are wired / torn down; is the concrete - /// instance the notifiers were wired against (a rebuild recreates the instance, so the entry is reset when - /// the instance changes). - private sealed record AlarmNotifierWiring(AlarmConditionState Alarm, List Folders); + /// event-notifier roots for it. The set (reference identity — review 01 P5: the + /// membership probe per wired folder was a linear ReferenceEquals walk) is mutated in place under + /// Lock as notifiers are wired / torn down; is the concrete + /// instance the notifiers were wired against (a rebuild recreates the + /// instance, so the entry is reset when the instance changes). + private sealed record AlarmNotifierWiring(AlarmConditionState Alarm, HashSet Folders); // This is a RECONCILE, not additive-only: it wires the supplied set AND unwires any // previously-tracked folder whose id is no longer in notifierFolderNodeIds @@ -1735,7 +1743,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // the entry (the old folder list referenced the discarded instance) when the instance differs. if (!_alarmNotifierWiring.TryGetValue(alarmKey, out var wiring) || !ReferenceEquals(wiring.Alarm, alarm)) { - wiring = new AlarmNotifierWiring(alarm, new List()); + wiring = new AlarmNotifierWiring( + alarm, new HashSet(System.Collections.Generic.ReferenceEqualityComparer.Instance)); _alarmNotifierWiring[alarmKey] = wiring; } @@ -1772,13 +1781,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // alarm.AddNotifier(isInverse:true, folder) — the upward bubble to the equipment folder // folder.AddNotifier(isInverse:false, alarm) — the downward AreEventsMonitored link // EnsureFolderIsEventNotifier(folder) — SubscribeToEvents + AddRootNotifier (idempotent) - // AddNotifier dedups by ReferenceEquals(Node), so re-wiring the same pair (idempotent re-apply) - // updates rather than duplicates. + // A pair this wiring already holds is skipped outright (review 01 P5): the SDK's AddNotifier + // dedups by a linear ReferenceEquals walk of the condition's notifier list, so re-wiring K + // folders on an idempotent re-apply cost O(K²) for nothing. + if (!wiring.Folders.Add(folder)) continue; alarm.AddNotifier(SystemContext, null, isInverse: true, folder); folder.AddNotifier(SystemContext, null, isInverse: false, alarm); EnsureFolderIsEventNotifier(folder); - - if (!wiring.Folders.Any(f => ReferenceEquals(f, folder))) wiring.Folders.Add(folder); } } } @@ -2826,10 +2835,18 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// The node whose subtree is forgotten. private void ForgetPredefinedSubtree(NodeState node) { - PredefinedNodes?.Remove(node.NodeId); + // Iterative with one reused child buffer — the recursive form allocated a list per node (review 01 P1). + var pending = new Stack(); + pending.Push(node); var children = new List(); - node.GetChildren(SystemContext, children); - foreach (var child in children) ForgetPredefinedSubtree(child); + while (pending.Count > 0) + { + var current = pending.Pop(); + PredefinedNodes?.Remove(current.NodeId); + children.Clear(); + current.GetChildren(SystemContext, children); + foreach (var child in children) pending.Push(child); + } } /// Promote to and @@ -2922,9 +2939,33 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 parent.AddChild(folder); AddPredefinedNode(SystemContext, folder); _folders[key] = folder; + if (!string.IsNullOrEmpty(parentNodeId)) IndexChild(MapKey(realm, parentNodeId), key); } } + /// Records under in the P2 subtree index. MUST be + /// called under Lock. Idempotent for the same pair; a child re-indexed under another parent moves. + private void IndexChild(RealmKey parent, RealmKey child) + { + if (_parentIndex.TryGetValue(child, out var previous)) + { + if (previous == parent) return; + UnindexChild(child); + } + + if (!_childIndex.TryGetValue(parent, out var children)) _childIndex[parent] = children = new HashSet(); + children.Add(child); + _parentIndex[child] = parent; + } + + /// Drops from the P2 subtree index. MUST be called under Lock. + private void UnindexChild(RealmKey child) + { + if (!_parentIndex.Remove(child, out var parent)) return; + if (_childIndex.TryGetValue(parent, out var children) && children.Remove(child) && children.Count == 0) + _childIndex.Remove(parent); + } + /// /// Update an EXISTING folder node's in place and notify /// subscribers, WITHOUT a rebuild — the in-place counterpart of for a @@ -3036,6 +3077,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 parent.AddChild(variable); AddPredefinedNode(SystemContext, variable); _variables[key] = variable; + if (!string.IsNullOrEmpty(parentFolderNodeId)) IndexChild(MapKey(realm, parentFolderNodeId), key); // Phase C: register the resolved historian tagname so the HistoryRead override can map this // NodeId back to its Aveva/historian source. A historized UNS reference node passes the SAME // tagname as its backing raw node — each NodeId keys its own entry, both mapping to one tagname. @@ -3500,16 +3542,6 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // TombstoneMonitoredNode). The shared root survives the rebuild and keeps its items. TombstoneAllMonitoredNodes(); - foreach (var v in _variables.Values) - { - v.Parent?.RemoveChild(v); - PredefinedNodes?.Remove(v.NodeId); - } - _variables.Clear(); - // Phase C: drop the NodeId→historian-tagname registrations alongside the variables they map. - _historizedTagnames.Clear(); - _arrayOnScalarRefused.Clear(); - // Capture every NATIVE condition's live state before the conditions are discarded, so the // re-materialise that follows in this same apply can put it back (see CaptureCarryOver). Scripted // conditions are not captured — their engine re-asserts them after the reload. @@ -3520,38 +3552,56 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 if (_nativeAlarmNodeIds.Contains(key)) _rebuildCarryOver[key] = CaptureCarryOver(alarm); } - foreach (var alarm in _alarmConditions.Values) - { - alarm.Parent?.RemoveChild(alarm); - ForgetPredefinedSubtree(alarm); - } // tear down every native-alarm→equipment-folder notifier pair bidirectionally // BEFORE the conditions + folders are cleared below, so no inverse-notifier entry leaks across the - // rebuild. The condition + folder NodeState objects still exist here (the loops above/below only - // detach them from their parents + PredefinedNodes), so RemoveNotifier is valid; then clear the - // tracking map so the re-materialise + re-wire on the next apply starts from a clean slate. + // rebuild. The condition + folder NodeState objects still exist here, so RemoveNotifier is valid; + // then clear the tracking map so the re-materialise + re-wire on the next apply starts clean. foreach (var alarmKey in _alarmNotifierWiring.Keys.ToList()) UnwireAlarmNotifiers(alarmKey); + // Review 01 P1 — detach whole subtrees, not leaves. NodeState.RemoveChild is a linear scan + + // RemoveAt, so detaching each of a folder's k children from it cost ~k²/2 compares under Lock + // (a 10k-tag device folder ≈ 50M). Everything this manager minted hangs under the shared root, + // which survives the rebuild, so detaching the root's own children once discards every subtree + // beneath them; the SDK lookup dictionary is still cleared per id (O(1) each) because + // PredefinedNodes indexes every node, condition children included. + if (_root is { } root) + { + var topLevel = new List(); + root.GetChildren(SystemContext, topLevel); + // Last-first: RemoveChild still scans for the reference, but RemoveAt on the tail shifts nothing. + for (var i = topLevel.Count - 1; i >= 0; i--) root.RemoveChild(topLevel[i]); + } + + foreach (var v in _variables.Values) PredefinedNodes?.Remove(v.NodeId); + _variables.Clear(); + // Phase C: drop the NodeId→historian-tagname registrations alongside the variables they map. + _historizedTagnames.Clear(); + _arrayOnScalarRefused.Clear(); + + foreach (var alarm in _alarmConditions.Values) ForgetPredefinedSubtree(alarm); _alarmConditions.Clear(); // H6a: drop the native-alarm flags in lock-step with the conditions they classify, so a // re-materialise on the next apply (possibly as the other kind) starts from a clean slate. _nativeAlarmNodeIds.Clear(); - foreach (var f in _folders.Values) - { - f.Parent?.RemoveChild(f); - PredefinedNodes?.Remove(f.NodeId); - } + foreach (var f in _folders.Values) PredefinedNodes?.Remove(f.NodeId); _folders.Clear(); + _childIndex.Clear(); + _parentIndex.Clear(); // Detach the Server↔folder HasNotifier ref for every promoted folder before dropping the // guard, otherwise the rebuild leaks an orphaned root-notifier reference on the Server - // object. RemoveRootNotifier just severs that link, so its order relative to the folder - // teardown above doesn't matter — but it must run under this same Lock. - foreach (var folder in _notifierFolders.Values) + // object. Done the way RemoveRootNotifier does it, but over the whole list at once: the SDK's + // per-folder removal scans RootNotifiers each time (O(F²) for F promoted folders). + if (RootNotifiers is { Count: > 0 } rootNotifiers) { - RemoveRootNotifier(folder); + foreach (var notifier in rootNotifiers) + { + notifier.OnReportEvent = null; + notifier.RemoveReference(ReferenceTypeIds.HasNotifier, true, ObjectIds.Server); + } + rootNotifiers.Clear(); } // Drop the notifier-folder guard so re-materialised alarms re-promote their (rebuilt) @@ -3611,6 +3661,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // caller (AddressSpaceApplier) falls back to a full rebuild (resync). Mirrors the per-node // teardown inside RebuildAddressSpace, scoped to this one id. if (!_variables.TryRemove(key, out var variable)) return false; + UnindexChild(key); TombstoneMonitoredNode(variable.NodeId, UtcNow); // 2026-08-15 review I3: sever the INVERSE half of every cross-tree Organizes edge this node holds // BEFORE detaching it — the same hazard (and the same fix shape) as UnwireAlarmNotifiers on a @@ -3678,6 +3729,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // lingering notifier folder is harmless (RemoveEquipmentSubtree demotes it when the whole // equipment goes). if (!_alarmConditions.TryRemove(key, out var condition)) return false; + UnindexChild(key); TombstoneMonitoredSubtree(condition, UtcNow); // unwire this condition's extra equipment-folder notifiers bidirectionally BEFORE // the condition drops — the folders (equipment, UNS realm) SURVIVE this scoped remove, so without @@ -3686,7 +3738,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 condition.Parent?.RemoveChild(condition); ForgetPredefinedSubtree(condition); _nativeAlarmNodeIds.Remove(key); - ReconcileAllEventsMonitoring(); + LowerAllEventsMonitoring(condition); e = BuildNodesRemovedModelChange(alarmNodeId, realm); } ReportModelChangeOutsideLock(e, alarmNodeId); @@ -3706,84 +3758,15 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // Unknown equipment id ⇒ map drift ⇒ false (caller rebuilds). if (!_folders.ContainsKey(equipKey)) return false; - // Maps key on the (realm, bare id) pair. Every descendant in the SAME realm shares that id exactly - // or begins with "/" (sub-folders, variables, condition nodes); the explicit realm - // compare is what makes the match realm-scoped — it replaces the old string key's leading "ns=N;s=" - // — so a node in the OTHER namespace that happens to share a bare id is never in scope. Snapshot - // each map's in-scope keys first (ToList) so we don't mutate while enumerating. - var prefix = equipmentNodeId + "/"; - bool InScope(RealmKey key) => key.Realm == realm - && (key.Id == equipmentNodeId || key.Id.StartsWith(prefix, StringComparison.Ordinal)); - - var now = UtcNow; - // Value variables (+ their historized-tagname registrations). - foreach (var id in _variables.Keys.Where(InScope).ToList()) - { - if (_variables.TryRemove(id, out var v)) - { - TombstoneMonitoredNode(v.NodeId, now); - // Sever the inverse half of each cross-tree Organizes edge - // first, exactly as RemoveVariableNode does — a UNS reference variable in scope points at a - // RAW node that survives this UNS-scoped removal and would otherwise keep a dangling edge. - SeverOutgoingReferences(v); - v.Parent?.RemoveChild(v); - PredefinedNodes?.Remove(v.NodeId); - } - _historizedTagnames.TryRemove(id, out _); - _arrayOnScalarRefused.TryRemove(id, out _); - } - - // Part 9 condition nodes (+ their native flags). - foreach (var id in _alarmConditions.Keys.Where(InScope).ToList()) - { - if (_alarmConditions.TryRemove(id, out var c)) - { - TombstoneMonitoredSubtree(c, now); - c.Parent?.RemoveChild(c); - ForgetPredefinedSubtree(c); - } - _nativeAlarmNodeIds.Remove(id); - } - ReconcileAllEventsMonitoring(); - - // a native alarm condition lives at its raw tag (Raw realm) and SURVIVES this - // UNS-scoped subtree removal, but it may hold an in-scope equipment folder as an extra - // event-notifier root. Unwire those condition↔folder pairs bidirectionally BEFORE the folders drop, - // else the surviving condition keeps a dangling inverse-notifier entry to a removed folder. (The - // condition's own scoped removal — if its raw tag is also removed this apply — runs later via - // RemoveAlarmConditionNode; this only reaches surviving conditions.) - foreach (var wiring in _alarmNotifierWiring.Values) - { - foreach (var folder in wiring.Folders.Where(f => InScope(MapKey(f.NodeId))).ToList()) - { - wiring.Alarm.RemoveNotifier(SystemContext, folder, bidirectional: true); - wiring.Folders.Remove(folder); - } - } - - // Notifier demotion BEFORE dropping the folders: sever the Server↔folder HasNotifier ref for - // every promoted folder in the subtree (an equipment folder, or a sub-folder that hosted an - // alarm), else the removal leaks an orphaned root-notifier reference on the Server object. - // _notifierFolders is keyed by the folder's NodeId; match InScope on its realm-qualified map key. - foreach (var kvp in _notifierFolders.Where(kv => InScope(MapKey(kv.Key))).ToList()) - { - RemoveRootNotifier(kvp.Value); - _notifierFolders.Remove(kvp.Key); - } - // Drop the event-history source registrations for the subtree (keyed by the folder id string). - foreach (var src in _eventNotifierSources.Keys.Where(InScope).ToList()) - _eventNotifierSources.TryRemove(src, out _); - - // Sub-folders + the equipment folder itself (children already detached above). - foreach (var id in _folders.Keys.Where(InScope).ToList()) - { - if (_folders.TryRemove(id, out var f)) - { - TombstoneMonitoredNode(f.NodeId, now); - f.Parent?.RemoveChild(f); - PredefinedNodes?.Remove(f.NodeId); - } - } + // Review 01 P2 — walk the parent→children index the materialise calls maintain instead of + // prefix-scanning every map (variables, conditions, folders, notifier folders, event sources and + // every alarm's wiring) per removed equipment, which made a PureRemove of M equipment O(M·N) + // under Lock. A node is detached from its SDK parent only when that parent SURVIVES (the + // equipment folder from its line; a node hung under the shared root because its parent was + // unknown at creation) — detaching k children from a folder that is itself being discarded cost + // k²/2 compares for nothing (NodeState.RemoveChild is a linear scan). + var removedParents = new HashSet(System.Collections.Generic.ReferenceEqualityComparer.Instance); + RemoveSubtree(equipKey, removedParents, UtcNow); e = BuildNodesRemovedModelChange(equipmentNodeId, realm); } @@ -3791,6 +3774,84 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 return true; } + /// Removes the node at and, when it is a folder, everything indexed beneath + /// it (children first). MUST be called under Lock. + /// The node to remove. + /// The folders already discarded by this removal — a node under one of them is + /// not detached from it. + /// The timestamp the removal statuses carry. + private void RemoveSubtree(RealmKey key, HashSet removedParents, DateTime now) + { + if (_folders.TryGetValue(key, out var folder)) + { + removedParents.Add(folder); + if (_childIndex.TryGetValue(key, out var children)) + { + foreach (var child in children.ToArray()) RemoveSubtree(child, removedParents, now); + _childIndex.Remove(key); + } + + _folders.TryRemove(key, out _); + UnindexChild(key); + TombstoneMonitoredNode(folder.NodeId, now); + + // A native alarm condition lives at its raw tag (Raw realm) and SURVIVES this UNS-scoped + // subtree removal, but it may hold this folder as an extra event-notifier root. Unwire those + // condition↔folder pairs bidirectionally BEFORE the folder drops, else the surviving condition + // keeps a dangling inverse-notifier entry to a removed folder. The folder's own notifier list + // names exactly the conditions wired into it (the forward half of each pair). + var notifiers = new List(); + folder.GetNotifiers(SystemContext, notifiers); + foreach (var entry in notifiers) + { + if (entry.IsInverse || entry.Node is not AlarmConditionState alarm) continue; + if (!_alarmNotifierWiring.TryGetValue(MapKey(alarm.NodeId), out var wiring) || !ReferenceEquals(wiring.Alarm, alarm)) continue; + alarm.RemoveNotifier(SystemContext, folder, bidirectional: true); + wiring.Folders.Remove(folder); + } + + // Notifier demotion BEFORE dropping the folder: sever the Server↔folder HasNotifier ref for a + // promoted folder (an equipment folder, or a sub-folder that hosted an alarm), else the removal + // leaks an orphaned root-notifier reference on the Server object. + if (_notifierFolders.Remove(folder.NodeId, out var promoted)) RemoveRootNotifier(promoted); + _eventNotifierSources.TryRemove(key, out _); + + if (folder.Parent is { } parent && !removedParents.Contains(parent)) parent.RemoveChild(folder); + PredefinedNodes?.Remove(folder.NodeId); + return; + } + + if (_variables.TryRemove(key, out var v)) + { + UnindexChild(key); + TombstoneMonitoredNode(v.NodeId, now); + // Sever the inverse half of each cross-tree Organizes edge first, exactly as RemoveVariableNode + // does — a UNS reference variable in scope points at a RAW node that survives this UNS-scoped + // removal and would otherwise keep a dangling edge. + SeverOutgoingReferences(v); + if (v.Parent is { } parent && !removedParents.Contains(parent)) parent.RemoveChild(v); + PredefinedNodes?.Remove(v.NodeId); + _historizedTagnames.TryRemove(key, out _); + _arrayOnScalarRefused.TryRemove(key, out _); + return; + } + + if (_alarmConditions.TryRemove(key, out var c)) + { + UnindexChild(key); + TombstoneMonitoredSubtree(c, now); + UnwireAlarmNotifiers(key); + if (c.Parent is { } parent && !removedParents.Contains(parent)) parent.RemoveChild(c); + ForgetPredefinedSubtree(c); + _nativeAlarmNodeIds.Remove(key); + LowerAllEventsMonitoring(c); + return; + } + + // Indexed but already gone (a surgical remove dropped it first) — nothing to do. + UnindexChild(key); + } + private FolderState ResolveParentFolder(string? parentNodeId, AddressSpaceRealm realm) { EnsureAddressSpaceCreated(); @@ -4296,10 +4357,18 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// children are monitorable in their own right). MUST be called under Lock. private void TombstoneMonitoredSubtree(NodeState node, DateTime now) { - TombstoneMonitoredNode(node.NodeId, now); + if (MonitoredNodes.Count == 0 && Volatile.Read(ref _suppressedItems) == 0) return; + var pending = new Stack(); + pending.Push(node); var children = new List(); - node.GetChildren(SystemContext, children); - foreach (var child in children) TombstoneMonitoredSubtree(child, now); + while (pending.Count > 0) + { + var current = pending.Pop(); + TombstoneMonitoredNode(current.NodeId, now); + children.Clear(); + current.GetChildren(SystemContext, children); + foreach (var child in children) pending.Push(child); + } } /// The rebuild form: every monitored node this manager serves except the shared root, which the @@ -4560,6 +4629,20 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 condition.SetAreEventsMonitored(SystemContext, true, includeChildren: true); } + /// + /// The removal-side mirror of (review 01 P3): gives back the one + /// condition's unit, touching no other condition. Call under Lock, for a condition being removed + /// or replaced. The surgical removal sites used to run the full reconcile — O(C) per removed condition + /// while any Server-object item was live, i.e. always in production. + /// + /// The condition leaving _alarmConditions. + private void LowerAllEventsMonitoring(AlarmConditionState condition) + { + AllEventsMonitoringConditionVisits++; + if (_allEventsMonitoredConditions.Remove(condition)) + condition.SetAreEventsMonitored(SystemContext, false, includeChildren: true); + } + /// /// How many conditions the Server-object monitored bookkeeping has visited since start — every /// condition a full walks, plus one per incremental diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerLockTimeBenchmarkTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerLockTimeBenchmarkTests.cs new file mode 100644 index 00000000..848529f3 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerLockTimeBenchmarkTests.cs @@ -0,0 +1,190 @@ +using System.Diagnostics; +using Microsoft.Extensions.Logging.Abstractions; +using Opc.Ua; +using Opc.Ua.Server; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// +/// Review 01 P1 / P2 / P3 — the lock-time budget of the three teardown paths that run under the +/// manager-wide SDK Lock on the pinned publish thread, where every Read / Browse / Publish waits. +/// +/// P1: a full rebuild of a 10k-tag device folder was O(k²) per folder because +/// NodeState.RemoveChild is a linear scan + RemoveAt per leaf (~50M compares). +/// P2: removing M equipment from an N-node space scanned every map per equipment (O(M·N)). +/// P3: every surgical condition removal re-walked every condition for the Server-object +/// monitoring bookkeeping (O(C) per removal) while any i=2253 item existed. +/// +/// The wall-clock budgets are generous for CI hardware; the before/after numbers are in the commit +/// message. P3 is pinned by the bookkeeping's own visit counter, which does not depend on the machine. +/// +[Trait("Category", "Benchmark")] +public sealed class NodeManagerLockTimeBenchmarkTests(ITestOutputHelper output) : IDisposable +{ + private static CancellationToken Ct => TestContext.Current.CancellationToken; + private static readonly TimeSpan Budget = TimeSpan.FromSeconds(1); + + private readonly string _pkiRoot = Path.Combine(Path.GetTempPath(), $"otopcua-bench-{Guid.NewGuid():N}"); + + /// P1 — a rebuild that discards one device folder holding 10k tags completes within the budget. + [Fact] + public async Task Rebuild_of_a_10k_tag_folder_stays_within_budget() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + const string device = "Plant/Modbus/dev1"; + nm.EnsureFolder(device, null, "dev1", AddressSpaceRealm.Raw); + for (var i = 0; i < 10_000; i++) + nm.EnsureVariable($"{device}/T{i}", device, $"T{i}", "Int32", writable: false, realm: AddressSpaceRealm.Raw); + nm.VariableCount.ShouldBe(10_000); + + var sw = Stopwatch.StartNew(); + nm.RebuildAddressSpace(); + sw.Stop(); + nm.CompleteAddressSpaceApply(); + + nm.VariableCount.ShouldBe(0); + nm.FolderCount.ShouldBe(0); + // Positive control: the space is really empty for the SDK too, and can be re-populated. + nm.GetManagerHandle(new NodeId($"{device}/T0", nm.NamespaceIndexForRealm(AddressSpaceRealm.Raw))).ShouldBeNull(); + nm.EnsureFolder(device, null, "dev1", AddressSpaceRealm.Raw); + nm.EnsureVariable($"{device}/T0", device, "T0", "Int32", writable: false, realm: AddressSpaceRealm.Raw); + nm.TryGetVariable($"{device}/T0", AddressSpaceRealm.Raw)!.Parent.ShouldBeSameAs(nm.TryGetFolder(device, AddressSpaceRealm.Raw)); + + output.WriteLine($"BENCH rebuild 10k-tag folder: {sw.ElapsedMilliseconds} ms"); + sw.Elapsed.ShouldBeLessThan(Budget, $"rebuild of a 10k-tag folder took {sw.ElapsedMilliseconds} ms"); + } + finally { await host.DisposeAsync(); } + } + + /// P2 — removing 200 equipment (250 signals each, 50k nodes) completes within the budget. + [Fact] + public async Task Removing_200_equipment_from_a_50k_node_space_stays_within_budget() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + const int equipmentCount = 200, perEquipment = 250; + for (var e = 0; e < equipmentCount; e++) + { + var equip = $"area/line/eq{e}"; + nm.EnsureFolder(equip, null, $"eq{e}", AddressSpaceRealm.Uns); + for (var i = 0; i < perEquipment; i++) + nm.EnsureVariable($"{equip}/S{i}", equip, $"S{i}", "Float", writable: false, realm: AddressSpaceRealm.Uns); + } + // A survivor the removals must not touch. + nm.EnsureFolder("area/line/keep", null, "keep", AddressSpaceRealm.Uns); + nm.EnsureVariable("area/line/keep/S0", "area/line/keep", "S0", "Float", writable: false, realm: AddressSpaceRealm.Uns); + nm.VariableCount.ShouldBe(equipmentCount * perEquipment + 1); + + var sw = Stopwatch.StartNew(); + for (var e = 0; e < equipmentCount; e++) + nm.RemoveEquipmentSubtree($"area/line/eq{e}", AddressSpaceRealm.Uns).ShouldBeTrue(); + sw.Stop(); + + nm.VariableCount.ShouldBe(1); + nm.FolderCount.ShouldBe(1); + nm.TryGetVariable("area/line/keep/S0").ShouldNotBeNull(); + nm.GetManagerHandle(new NodeId("area/line/eq0/S0", nm.NamespaceIndexForRealm(AddressSpaceRealm.Uns))).ShouldBeNull(); + + output.WriteLine($"BENCH remove 200 equipment / 50k nodes: {sw.ElapsedMilliseconds} ms"); + sw.Elapsed.ShouldBeLessThan(Budget, $"removing 200 equipment took {sw.ElapsedMilliseconds} ms"); + } + finally { await host.DisposeAsync(); } + } + + /// + /// P3 — with a Server-object item live, a surgical condition removal lowers only that condition's + /// monitoring unit instead of re-walking every condition: the bookkeeping visits at most one + /// condition per removal. (A full reconcile visits every condition twice — desired + held.) + /// + [Fact] + public async Task A_surgical_condition_removal_visits_only_its_own_condition() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureFolder("eq-1", null, "Equipment 1", AddressSpaceRealm.Uns); + const int conditions = 50; + for (var i = 0; i < conditions; i++) + nm.MaterialiseAlarmCondition($"alm-{i}", "eq-1", $"Alarm {i}", "OffNormalAlarm", 500, AddressSpaceRealm.Uns); + + // A Server-object (all-events) subscriber, so the per-condition monitoring unit is in play. + nm.SubscribeToAllEvents( + new OperationContext(new RequestHeader(), secureChannelContext: null, RequestType.CreateMonitoredItems, identity: null), + 1, new StubEventItem(), unsubscribe: false); + nm.ConditionEventsMonitored("alm-0", AddressSpaceRealm.Uns).ShouldBeTrue("non-vacuity: the units really were raised"); + + var before = nm.AllEventsMonitoringConditionVisits; + for (var i = 0; i < 10; i++) + nm.RemoveAlarmConditionNode($"alm-{i}", AddressSpaceRealm.Uns).ShouldBeTrue(); + var visits = nm.AllEventsMonitoringConditionVisits - before; + output.WriteLine($"BENCH all-events visits for 10 condition removals over {conditions} conditions: {visits}"); + + visits.ShouldBeLessThanOrEqualTo(10, $"ten removals visited {visits} conditions; a full reconcile per removal would visit ~{10 * 2 * conditions}"); + // The survivors keep their unit. + nm.ConditionEventsMonitored("alm-49", AddressSpaceRealm.Uns).ShouldBeTrue(); + } + finally { await host.DisposeAsync(); } + } + + private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync() => + await OpcUaHostStart.StartAsync( + port => new OpcUaApplicationHost( + new OpcUaApplicationHostOptions + { + ApplicationName = "OtOpcUa.BenchTest", + ApplicationUri = $"urn:OtOpcUa.BenchTest:{Guid.NewGuid():N}", + OpcUaPort = port, + PublicHostname = "localhost", + PkiStoreRoot = _pkiRoot, + }, + NullLogger.Instance), + () => new OtOpcUaSdkServer(), Ct); + + public void Dispose() + { + if (Directory.Exists(_pkiRoot)) Directory.Delete(_pkiRoot, recursive: true); + } + + /// The minimum of a Server-object event item the all-events bookkeeping touches: identity only. + private sealed class StubEventItem : IEventMonitoredItem + { + public bool MonitoringAllEvents => true; + public NodeId NodeId => ObjectIds.Server; + public IUserIdentity EffectiveIdentity => null!; + public object ManagerHandle => null!; + public void QueueEvent(IFilterTarget instance) { } + public void QueueEvent(IFilterTarget instance, bool bypassFilter) { } + public void Dispose() { } + + public INodeManager NodeManager => throw new NotSupportedException(); + public ISession Session => throw new NotSupportedException(); + public uint Id => 1; + public uint SubscriptionId => 1; + public bool IsDurable => false; + public uint ClientHandle => 1; + public ISubscription SubscriptionCallback { get => throw new NotSupportedException(); set => throw new NotSupportedException(); } + public int MonitoredItemType => MonitoredItemTypeMask.Events; + public bool IsReadyToPublish => false; + public bool IsReadyToTrigger { get => false; set { } } + public bool IsResendData => false; + public MonitoringMode MonitoringMode => MonitoringMode.Reporting; + public double SamplingInterval => 0; + public EventFilter EventFilter => null!; + public void SetupResendDataTrigger() { } + public ServiceResult GetCreateResult(out MonitoredItemCreateResult result) => throw new NotSupportedException(); + public ServiceResult GetModifyResult(out MonitoredItemModifyResult result) => throw new NotSupportedException(); + public IStoredMonitoredItem ToStorableMonitoredItem() => throw new NotSupportedException(); + public MonitoringMode SetMonitoringMode(MonitoringMode monitoringMode) => throw new NotSupportedException(); + public bool Publish(OperationContext context, Queue notifications, uint maxNotificationsPerPublish) => throw new NotSupportedException(); + public ServiceResult ModifyAttributes(DiagnosticsMasks diagnosticsMasks, TimestampsToReturn timestampsToReturn, uint clientHandle, MonitoringFilter originalFilter, MonitoringFilter filterToUse, Opc.Ua.Range range, double samplingInterval, uint queueSize, bool discardOldest) => throw new NotSupportedException(); + } +}