From aec5ce565a42ba961ebc77b86f10ec46319f9003 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 13:10:55 -0400 Subject: [PATCH] docs: record value-pipeline batching (plan 2026-08-15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs/v2/Galaxy.Performance.md gains a "Value-path batching (2026-08-15)" section: the batch seams (IBatchSubscribable / PollGroupEngine onBatch -> AttributeValueBatchPublished -> AttributeValueBatchUpdate -> sink WriteValues with 512-chunked single-lock batches in the node manager), the invariants (#519 status codes verbatim, per-ITEM metering, Modbus deadband honored via the driver-filtered buffer, the ValueWriteBatchException partial-failure contract), the RealmKey re-key, and the pinned dispatcher's throughput 1 -> 100. - The plan doc gains a "Follow-ups (recorded, not implemented)" section: the PollGroupEngine unconditional-collection trap (a future per-tag filter in S7/AbCip/FOCAS would silently reintroduce the deadband-bypass bug), a two-level driverId -> (rawPath -> refs) routing map, null/empty guard consistency on the internal batch messages, a possible DriverHostActorLiveValueTests merge, and the pre-existing aborted-cycle tail-loss on the three unbuffered drivers. - tasks.json: all 13 tasks completed. CLAUDE.md needed no edit: its only statement in this area ("One driver publish for a RawPath fans (in DriverHostActor) to the raw NodeId AND every referencing UNS NodeId") is still true — batching changed the message granularity, not the fan-out semantics. Verification: dotnet build ZB.MOM.WW.OtOpcUa.slnx clean (0 errors, no new warning classes, zero warnings in any production file). 9 unit projects green, 3470 passed / 5 skipped / 0 failed: Core.Abstractions 305, Core 351+1s, Commons 348, Runtime 575+3s, OpcUaServer 424+1s, Modbus 399, S7 302, AbCip 410, FOCAS 356. No DB-backed suite was run — tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests does not exist, and nothing this branch touches is covered by the SQL-backed projects. --- .../2026-08-15-value-pipeline-batching.md | 38 +++++++++++++ ...8-15-value-pipeline-batching.md.tasks.json | 28 +++++----- docs/v2/Galaxy.Performance.md | 53 +++++++++++++++++++ 3 files changed, 105 insertions(+), 14 deletions(-) diff --git a/docs/plans/2026-08-15-value-pipeline-batching.md b/docs/plans/2026-08-15-value-pipeline-batching.md index 8dd51a71..218c346d 100644 --- a/docs/plans/2026-08-15-value-pipeline-batching.md +++ b/docs/plans/2026-08-15-value-pipeline-batching.md @@ -591,3 +591,41 @@ Steps: - **No MQTT / MTConnect / Sparkplug / Galaxy / OpcUaClient batch implementations** — they flow through the single-item-batch path unchanged. - **No change to alarm paths** (`AttributeAlarmPublished`, `AlarmStateUpdate`) — value path only. - **No PollGroupEngine cadence change** (fixed-rate scheduling is a separate item). + +--- + +## Follow-ups (recorded, not implemented) + +Raised during Tasks 1–12 review and deliberately left out of this plan's scope. None is a defect in the +delivered work; each is a place where the next change in the area could go wrong quietly. + +1. **`PollGroupEngine` collects batch items unconditionally — the batch event is only as trustworthy as + the driver's `onChange`.** The engine has no knowledge of a driver's publish filter, so its items list + is pre-filter by construction. That is safe *today* only because S7, AbCip and FOCAS forward every + `onChange` unconditionally, and Modbus — the one driver with a deadband — ignores the engine's list and + raises from its own filtered buffer. The trap: a future per-tag filter added to S7/AbCip/FOCAS's + `onChange` would silently reintroduce the exact deadband-bypass class of bug fixed for Modbus in + `cd4ce305`, with a green test suite (the two events would simply disagree, and only a consumer + preferring the batch would notice). Consider changing the engine's `onChange` contract to + `Func<..., bool>` so the engine collects an item only when the driver reports it published — making the + filter structurally impossible to skip rather than a convention each driver must remember. +2. **Two-level `driverId → (rawPath → refs)` routing map in `DriverHostActor`.** The fan-out map is keyed + by a composite of driver id + raw path, so resolving a batch re-hashes the driver id once per item even + though a batch is by definition single-driver. Nesting the map would hash the driver id once per batch + and the raw path once per item. Measure first — batches are small enough that this may be noise + relative to the message-count win already banked. +3. **Null/empty guard consistency on the internal batch messages.** The guards on + `AttributeValueBatchPublished` / `AttributeValueBatchUpdate` grew per task and are not uniform — some + seams drop an empty batch early, others tolerate it downstream. These are internal messages with + trusted senders so nothing is unsafe today; worth one pass to make the contract identical at every hop + and stated once, rather than re-derived per reader. +4. **Possible merge of `DriverHostActorLiveValueTests` into the batch suite.** After Task 8 both files + drive the same fan-out through the same message; the split is now historical rather than meaningful. + Merging would remove a genuine ambiguity about where a new fan-out test belongs — but check first that + the older suite's per-item comms-loss cases really are covered, since those enter via a different seam. +5. **Aborted-cycle tail-loss when a per-tag handler throws, for the three unbuffered drivers.** If a + consumer's `onChange` throws mid-cycle in S7/AbCip/FOCAS, the cycle aborts and the items already + collected for that cycle never reach a batch raise. **Pre-existing and narrower than the Modbus case** + (Modbus's own buffer is per-handle and is dropped on unsubscribe/dispose, so it cannot leak), and it + predates this plan — the per-tag path lost the same tail before batching existed. Recorded so it is not + mistaken for a regression introduced here. diff --git a/docs/plans/2026-08-15-value-pipeline-batching.md.tasks.json b/docs/plans/2026-08-15-value-pipeline-batching.md.tasks.json index 87f1ff1b..f90c182e 100644 --- a/docs/plans/2026-08-15-value-pipeline-batching.md.tasks.json +++ b/docs/plans/2026-08-15-value-pipeline-batching.md.tasks.json @@ -1,19 +1,19 @@ { "planPath": "docs/plans/2026-08-15-value-pipeline-batching.md", "tasks": [ - {"id": 1, "subject": "Task 1: Batch contract types in Core.Abstractions", "status": "pending"}, - {"id": 2, "subject": "Task 2: PollGroupEngine batch callback", "status": "pending", "blockedBy": [1]}, - {"id": 3, "subject": "Task 3a: Modbus + S7 raise the batch event", "status": "pending", "blockedBy": [2]}, - {"id": 4, "subject": "Task 3b: AbCip + FOCAS raise the batch event", "status": "pending", "blockedBy": [2]}, - {"id": 5, "subject": "Task 4: DriverInstanceActor batch forwarding", "status": "pending", "blockedBy": [1]}, - {"id": 6, "subject": "Task 5: ValueWrite + sink WriteValues default method", "status": "pending"}, - {"id": 7, "subject": "Task 6: OtOpcUaNodeManager batched write under one lock", "status": "pending", "blockedBy": [6]}, - {"id": 8, "subject": "Task 7: OpcUaPublishActor batch message", "status": "pending", "blockedBy": [6]}, - {"id": 9, "subject": "Task 8: DriverHostActor batched fan-out", "status": "pending", "blockedBy": [5, 8]}, - {"id": 10, "subject": "Task 9: DependencyMuxActor batch receive", "status": "pending", "blockedBy": [9]}, - {"id": 11, "subject": "Task 10: Raise pinned-dispatcher throughput", "status": "pending"}, - {"id": 12, "subject": "Task 11: Re-key node-manager maps", "status": "pending", "blockedBy": [7]}, - {"id": 13, "subject": "Task 12: Full verification + docs", "status": "pending", "blockedBy": [3, 4, 9, 10, 11, 12]} + {"id": 1, "subject": "Task 1: Batch contract types in Core.Abstractions", "status": "completed"}, + {"id": 2, "subject": "Task 2: PollGroupEngine batch callback", "status": "completed", "blockedBy": [1]}, + {"id": 3, "subject": "Task 3a: Modbus + S7 raise the batch event", "status": "completed", "blockedBy": [2]}, + {"id": 4, "subject": "Task 3b: AbCip + FOCAS raise the batch event", "status": "completed", "blockedBy": [2]}, + {"id": 5, "subject": "Task 4: DriverInstanceActor batch forwarding", "status": "completed", "blockedBy": [1]}, + {"id": 6, "subject": "Task 5: ValueWrite + sink WriteValues default method", "status": "completed"}, + {"id": 7, "subject": "Task 6: OtOpcUaNodeManager batched write under one lock", "status": "completed", "blockedBy": [6]}, + {"id": 8, "subject": "Task 7: OpcUaPublishActor batch message", "status": "completed", "blockedBy": [6]}, + {"id": 9, "subject": "Task 8: DriverHostActor batched fan-out", "status": "completed", "blockedBy": [5, 8]}, + {"id": 10, "subject": "Task 9: DependencyMuxActor batch receive", "status": "completed", "blockedBy": [9]}, + {"id": 11, "subject": "Task 10: Raise pinned-dispatcher throughput", "status": "completed"}, + {"id": 12, "subject": "Task 11: Re-key node-manager maps", "status": "completed", "blockedBy": [7]}, + {"id": 13, "subject": "Task 12: Full verification + docs", "status": "completed", "blockedBy": [3, 4, 9, 10, 11, 12]} ], - "lastUpdated": "2026-08-15T00:00:00Z" + "lastUpdated": "2026-08-15T17:08:35Z" } diff --git a/docs/v2/Galaxy.Performance.md b/docs/v2/Galaxy.Performance.md index d83e60a8..7fabe0a2 100644 --- a/docs/v2/Galaxy.Performance.md +++ b/docs/v2/Galaxy.Performance.md @@ -181,3 +181,56 @@ Safety: reuse is serialised under `_evalGate`, so two threads can never observe - `Reevaluation_reuses_the_same_read_cache_dictionary` — asserts dictionary instance identity across two evaluations. - `Reevaluation_reuses_the_same_predicate_context` — same, for the context. - `LoadAsync_drops_the_prior_generations_scratch` — asserts a publish resets the scratch. + +## Value-path batching (2026-08-15) + +The hot value path is batched end to end: one message per driver production burst (a poll cycle, an +inbound message, a stream chunk) instead of one per changed tag at every hop, one SDK-lock acquisition +per batch instead of one per value, and no per-write key allocation in the node manager. Nothing about +*what* is published changed — only how many messages and lock takes it costs. Plan + +task-by-task rationale: [`docs/plans/2026-08-15-value-pipeline-batching.md`](../plans/2026-08-15-value-pipeline-batching.md). + +**The seams, in order:** + +- **`IBatchSubscribable` / `PollGroupEngine`'s `onBatch`** — an optional driver capability raising + `OnDataChangeBatch` once per burst. The four engine-based poll drivers (Modbus, S7, AbCip, FOCAS) get + it from the engine's batch callback. The per-tag `ISubscribable.OnDataChange` is unchanged and still + raised — Galaxy cold reads and the driver CLIs subscribe it. A consumer subscribes to **exactly one** + of the two; subscribing both double-delivers. +- **`DriverInstanceActor` → `AttributeValueBatchPublished`** — one per burst, preferring the batch event + when the driver implements `IBatchSubscribable` and wrapping a non-batch driver's per-tag events as + single-item batches, so downstream sees one shape. The detach path tears the batch handler down too + (load-bearing: the `IDriver` outlives the actor across host respawns). +- **`DriverHostActor` → `AttributeValueBatchUpdate`** — resolves the whole batch's `NodeRealmRef`s + (raw NodeId + every referencing UNS NodeId) and sends **one** message to `OpcUaPublishActor`, plus one + batch Tell to the dependency mux. The mux keeps its per-item receive — the comms-loss fan-out + fabricates per-item messages. +- **`OtOpcUaNodeManager.WriteValues`** — the sink's batch override takes the manager-wide `Lock` once per + chunk of **512** items rather than once per value, with each item isolated so one bad node cannot lose + the rest of the batch. + +**Invariants the batching preserves — do not break them:** + +- **#519 status codes ride verbatim.** The full `uint` status code, sub-code included, reaches the SDK + unchanged at every hop; batching moved the values, it did not re-derive them. Timestamps likewise come + from the driver — nothing on this path stamps `DateTime.UtcNow`. +- **Metering stays per ITEM, not per batch** (`OpcUaPublishActor`'s write tally + the + `OtOpcUaTelemetry.OpcUaSinkWrite` meter). Counting per message would have silently dropped the + dashboards by up to 100×. +- **Deadband is honored on the batch event.** ⚠️ `PollGroupEngine` collects its batch list + **unconditionally** — it has no knowledge of a driver's publish filter — so `ModbusDriver` ignores the + engine's list and raises from its own buffer of what actually passed `ShouldPublish`, using `onBatch` + purely as the end-of-cycle signal. Forwarding the engine's list instead would carry changes the per-tag + event suppressed, and a consumer preferring the batch event would see deadband silently stop working. +- **Partial failure is typed, not swallowed.** A sink that applies some of a batch and fails the rest + reports `ValueWriteBatchException` (first failed NodeId + failed/succeeded counts); the publish actor's + typed catch advances the tally by the **succeeded** count — not zero (the general catch's behaviour) and + not the whole batch. + +**Two supporting changes:** the node manager's variable / folder / alarm / historized-tagname maps are +re-keyed by a `RealmKey(Realm, Id)` struct, so a write no longer allocates `new NodeId(id, ns).ToString()` +per value (the realm still travels explicitly — it is never parsed back out of an id string). And the +pinned `opcua-synchronized-dispatcher`'s `throughput` rose **1 → 100**: the actor keeps its own dedicated +thread (that is what the `PinnedDispatcher` guarantees, not `throughput`), so this only removes a mailbox +re-schedule round-trip per message on the hottest actor in the process, with zero change to concurrency. +Keep it in sync with the inline HOCON copy in `ServiceCollectionExtensions.WithOtOpcUaRuntimeActors`.