From 0edc3b55a6195fb7be3380aeddd0107041669826 Mon Sep 17 00:00:00 2001 From: romanett Date: Tue, 20 Oct 2026 11:01:19 +0200 Subject: [PATCH] Server: allocate data change notifications instead of pooling them (#4830) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #4829 (publish timer sweep): this PR targets its branch, so the diff shows only this change. Retarget to master once #4829 merges. MonitoredItem.Publish rented every MonitoredItemNotification from the process-wide activator pool (BoundedObjectPool, bound 2 x monitored items, capped at 65536) and NotificationMessageLease returned it once the message was sent and acknowledged. Under load the pool costs more CPU than it saves: - A notification lives until its message is acknowledged, so a whole publishing cycle of notifications is in flight at once. With the publish sweep finishing within a few ms (fixed cadence + parallel remainder) the in-flight burst is far above the cap and most Gets miss anyway (MonitoredItemNotification 6 % -> 45 % of server allocation). - Raising the cap to 1M made it worse: 5.5 vs 4.4 us per notification, lock contention 16k vs 5k per window, delivery 93 vs 95.5 %. - Every hit touches an instance that went cold in the ~100-200 ms since it was returned, Reuse touches it once more, and the shared queue and counters bounce between cores. A fresh allocation comes from the thread's hot allocation context. The notifications are now allocated in MonitoredItem.Publish and left to the GC; RecyclePayload no longer reuses them, and the pool bound is only raised for EventFieldList. NotificationMessage, DataChangeNotification, EventNotificationList and EventFieldList pooling and the rented MonitoredItemNotification[] arrays are unchanged. Paired A/B on top of the publish sweep fix (perf/ServerLoadHarness, encrypted, alternating rounds, medians; runs repeated while processes outside the harness used more than 1.5 cores): | shape (sessions x subs x items) | GC | n | us/notif | server cores | delivered | GC pause / 15 s | p99 write->notify | |---------------------------------|--------|---|------------|--------------|--------------|-----------------|-------------------| | 50 x 15 x 600, pub 100, w 150 | server | 2 | 4.4 -> 3.1 | 11.9 -> 8.2 | 89 -> 89 % | 0.8 -> 1.5 s | 278 -> 325 ms | | 10 x 15 x 600, pub 100, w 150 | server | 3 | 5.1 -> 3.3 | 2.9 -> 2.0 | 100 -> 100 % | 15 -> 88 ms | 137 -> 141 ms | | 1000 x 1 x 1, pub 1000, w 500 | server | 7 | 158 -> 190 | 0.16 -> 0.19 | 50 -> 50 % | 15 -> 28 ms | 480 -> 490 ms | | 2 x 1 x 100, pub 100, w 150 | server | 3 | 59 -> 47 | 0.08 -> 0.06 | 100 -> 100 % | 7 -> 6 ms | 103 -> 114 ms | | 50 x 15 x 600, pub 100, w 150 | ws | 3*| 4.5 -> 3.5 | 10.5 -> 7.0 | 86 -> 96 % | 5.1 -> 8.6 s | 447 -> 373 ms | | 10 x 15 x 600, pub 100, w 150 | ws | 5*| 5.4 -> 3.6 | 3.3 -> 1.9 | 100 -> 100 % | 0.45 -> 1.1 s | 144 -> 155 ms | (*) workstation GC rows combine the A/B with the switch-build runs of the same comparison. Excluded: six runs (both builds) in which the 50-session shape collapsed with the server mostly idle (up to 16.7 s GC pause in the 15 s window, 0-4 % delivered), the GC suspension stall pattern seen before on this machine under external CPU saturation. The 1000-session cores differ by 0.03 with a same-build spread of 0.13-0.25 at ~1000 notifications/s. Trade-off: CPU per notification drops 25-40 % wherever notifications flow, but short-lived notifications now die young: gen0 GCs per 15 s 2 -> 24 (server GC, 10 sessions), gen1 35 -> 154 (workstation GC, 10 sessions), so GC pause time rises about 2x, and p99 latency rises with workstation GC at moderate load (+8 % at 10 sessions). When pooling could pay off again is tracked in #4819. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: Marc Schier --- .../Subscription/MonitoredItem/MonitoredItem.cs | 14 +++++++++----- .../Subscription/NotificationMessageLease.cs | 11 +++-------- src/Opc.Ua.Server/Subscription/Subscription.cs | 14 +++++++------- .../NotificationEncodingLifetimeRegressionTests.cs | 9 +++++++-- .../Subscription/ReadyListRegressionTests.cs | 8 ++------ 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/Opc.Ua.Server/Subscription/MonitoredItem/MonitoredItem.cs b/src/Opc.Ua.Server/Subscription/MonitoredItem/MonitoredItem.cs index b0b4412d99..dc2be30b45 100644 --- a/src/Opc.Ua.Server/Subscription/MonitoredItem/MonitoredItem.cs +++ b/src/Opc.Ua.Server/Subscription/MonitoredItem/MonitoredItem.cs @@ -2362,11 +2362,15 @@ namespace Opc.Ua.Server value = value.WithSourceTimestamp(DateTimeUtc.MinValue); } - // the notification is pooled; it is returned to the pool when the message that - // carries it was acknowledged and sent (NotificationMessageLease). - var item = (MonitoredItemNotification)MonitoredItemNotificationActivator.Instance.CreateInstance(); - item.ClientHandle = ClientHandle; - item.Value = value; + // not pooled: a notification lives until its message was sent and acknowledged, so + // a publishing cycle holds all notifications of the cycle at once. Recycling them + // through the shared activator pool cost more CPU (pool contention, touching cold + // instances again) than allocating them. + var item = new MonitoredItemNotification + { + ClientHandle = ClientHandle, + Value = value + }; ServerUtils.ReportPublishValue(NodeId, Id, value); notifications.Enqueue(item); diff --git a/src/Opc.Ua.Server/Subscription/NotificationMessageLease.cs b/src/Opc.Ua.Server/Subscription/NotificationMessageLease.cs index c4055a37e0..2f6c17af7e 100644 --- a/src/Opc.Ua.Server/Subscription/NotificationMessageLease.cs +++ b/src/Opc.Ua.Server/Subscription/NotificationMessageLease.cs @@ -36,8 +36,7 @@ namespace Opc.Ua.Server /// /// Counts the owners of a notification message whose payload objects (the /// , its notification lists and their - /// / entries) came - /// from their activator pools. The payload is returned to the pools only when the last + /// entries) came from their activator pools. The payload is returned to the pools only when the last /// owner releases it. /// /// @@ -126,12 +125,8 @@ namespace Opc.Ua.Server { if (data[i].TryGetValue(out DataChangeNotification? dcn)) { - ReadOnlySpan items = - dcn.MonitoredItems.Span; - for (int j = 0; j < items.Length; j++) - { - (items[j] as IPooledEncodeable)?.Reuse(); - } + // the MonitoredItemNotification entries are not pooled (see + // MonitoredItem.Publish) and are left to the garbage collector. (dcn as IPooledEncodeable)?.Reuse(); } else if (data[i].TryGetValue(out EventNotificationList? enl)) diff --git a/src/Opc.Ua.Server/Subscription/Subscription.cs b/src/Opc.Ua.Server/Subscription/Subscription.cs index bfa3ed2a1d..fb78321493 100644 --- a/src/Opc.Ua.Server/Subscription/Subscription.cs +++ b/src/Opc.Ua.Server/Subscription/Subscription.cs @@ -1113,15 +1113,16 @@ namespace Opc.Ua.Server /// /// Adds the monitored items of this subscription to the process-wide count that sizes - /// the notification pools, and raises the pool bounds when the count grew. + /// the event field list pool, and raises the pool bound when the count grew. /// /// - /// The activator pools are process-wide and retain 1024 instances by default. A - /// notification is recycled after its message was sent and acknowledged, so about two - /// publishing cycles of notifications are in flight; with more items than the bound - /// most notifications would be allocated again. The bound follows twice the number of + /// The activator pool is process-wide and retains 1024 instances by default. An + /// event field list is recycled after its message was sent and acknowledged, so about + /// two publishing cycles of event field lists are in flight; with more items than the + /// bound most of them would be allocated again. The bound follows twice the number of /// monitored items of all subscriptions in the process, capped at - /// , and is never lowered. + /// , and is never lowered. Data change + /// notifications are not pooled (see ). /// private void CountPooledItems(int delta) { @@ -1137,7 +1138,6 @@ namespace Opc.Ua.Server int bound = (int)Math.Min( kMaxPooledNotifications, Math.Max(PooledEncodeableType.DefaultMaximumRetained, total * 2)); - MonitoredItemNotificationActivator.Instance.EnsureMaximumRetained(bound); EventFieldListActivator.Instance.EnsureMaximumRetained(bound); } } diff --git a/tests/Opc.Ua.Server.Tests/Subscription/NotificationEncodingLifetimeRegressionTests.cs b/tests/Opc.Ua.Server.Tests/Subscription/NotificationEncodingLifetimeRegressionTests.cs index 172d6459bf..9bad0a3b0d 100644 --- a/tests/Opc.Ua.Server.Tests/Subscription/NotificationEncodingLifetimeRegressionTests.cs +++ b/tests/Opc.Ua.Server.Tests/Subscription/NotificationEncodingLifetimeRegressionTests.cs @@ -178,7 +178,10 @@ namespace Opc.Ua.Server.Tests Is.True); Assert.That(value!.MonitoredItems[0].ClientHandle, Is.EqualTo(77u)); Assert.That(value.MonitoredItems[0].Value.WrappedValue.GetInt32(), Is.EqualTo(42)); - Assert.That(item.ClientHandle, Is.Zero); + + // data change notifications are not pooled by the server; they are left to the GC. + Assert.That(item.ClientHandle, Is.EqualTo(77u)); + item.Reuse(); eventItem.Reuse(); } queue.Clear(); @@ -212,7 +215,9 @@ namespace Opc.Ua.Server.Tests Assert.Multiple(() => { Assert.That(message.ReuseCount, Is.EqualTo(1)); - Assert.That(item.ReuseCount, Is.EqualTo(1)); + + // data change notifications are not pooled by the server and never reused. + Assert.That(item.ReuseCount, Is.Zero); }); } diff --git a/tests/Opc.Ua.Server.Tests/Subscription/ReadyListRegressionTests.cs b/tests/Opc.Ua.Server.Tests/Subscription/ReadyListRegressionTests.cs index 8815613ebe..c6a5f2bb0e 100644 --- a/tests/Opc.Ua.Server.Tests/Subscription/ReadyListRegressionTests.cs +++ b/tests/Opc.Ua.Server.Tests/Subscription/ReadyListRegressionTests.cs @@ -198,16 +198,12 @@ namespace Opc.Ua.Server.Tests } [Test] - public async Task CreatingMonitoredItemsRaisesTheNotificationPoolBoundAsync() + public async Task CreatingMonitoredItemsRaisesTheEventFieldListPoolBoundAsync() { // the pools are process-wide and other tests may only raise them further. using var scenario = new Scenario(withScanItem: false, maxNotificationsPerPublish: 0, maxMessageCount: 100); await scenario.CreateAsync(itemCount: 1200).ConfigureAwait(false); - Assert.Multiple(() => - { - Assert.That(MonitoredItemNotificationActivator.Instance.MaximumRetained, Is.GreaterThanOrEqualTo(2400)); - Assert.That(EventFieldListActivator.Instance.MaximumRetained, Is.GreaterThanOrEqualTo(2400)); - }); + Assert.That(EventFieldListActivator.Instance.MaximumRetained, Is.GreaterThanOrEqualTo(2400)); } private static async Task AssertSameAsScanAsync(