Feature/optimize 06 struct by symbol browse fix

docs/batch-read-write-performance-optimization.md

@@ -0,0 +1,124 @@
+# S7CommPlus Batch Read/Write Performance Optimization
+
+Notes on the analysis and optimizations made to the batched read/write path of the
+S7CommPlus driver, targeted at scenarios with a very large number of tags.
+
+## Context / problem statement
+
+The optimization was driven by a real-world case:
+
+- **~210,000 tags** read in a single logical operation.
+- `_tagsPerReadRequestMax = 100`, i.e. the PLC negotiates a maximum of **100 items per
+  read request**.
+- Requirement: strictly preserve the existing **hard-abort-on-error** behavior. Partial
+  results are deliberately *not* surfaced to the caller (the protocol layer produces
+  them, but the client layer intentionally discards them via `ThrowIfError`). Only
+  performance may change, not observable behavior.
+
+## Where the time goes (baseline analysis)
+
+Key methods:
+
+- `S7CommPlusClient.ReadTagValuesInBatches` (client-side outer batching loop)
+- `S7CommPlusProtocolSession.ReadValues` / `WriteValues` (protocol-layer request building)
+- `GetSerializedRequestLengthForBatching` (payload-size probe)
+- `ExceedsSingleFramePayload` (comparison against the negotiated single-frame payload limit)
+
+Findings:
+
+1. **2100 sequential requests.** With 100 items per request, 210,000 tags turn into
+   2100 sequential request/response round-trips. This is dominated by the PLC item limit,
+   so it is mostly I/O-bound and hard to change without protocol-level pipelining (see
+   backlog item 6).
+
+2. **Two nested batching layers.** The client batches into `_tagsPerReadRequestMax`
+   chunks, and the protocol layer *again* splits by the negotiated TPDU payload size.
+
+3. **O(n^2) payload check (the main avoidable CPU cost).** For every candidate item that
+   was added, the *entire* request was fully re-serialized to measure its byte length.
+   Building one chunk of `n` items therefore cost `O(n)` full serializations, i.e.
+   `O(n^2)` byte-level work per chunk. Across 2100 chunks this dominated the CPU time.
+
+## Optimizations implemented
+
+### 1. Remove the O(n^2) reserialization (committed)
+
+- Branch `feature/perf-fix-batching-payload-check`, commit `f0bccad`.
+- New helper `AppendItemsWithinPayloadLimit` in `S7CommPlusProtocolSession.cs`, used by
+  both the read path (`ReadValues`) and the write path (`CreateWriteRequestBatch`).
+- Technique: **galloping search followed by binary search** to find the exact number of
+  items that fit within the payload limit, using **O(log n)** full-request serializations
+  per chunk instead of O(n).
+- Correctness basis: the serialized request length **grows monotonically** with the item
+  count (each item contributes at least one byte; VLQ-encoded counters never shrink as
+  items are added). This makes the search find exactly the same maximum as the original
+  linear scan.
+- Preserved semantics: the **first item of a chunk is always included unconditionally**
+  (matching the legacy "never reject the first item" behavior), even if that item alone
+  would exceed the limit.
+- Regression test added: `WriteBatchSplitPointMatchesLinearScanForVariousSizes`
+  (12 cases across power-of-two boundaries) proves identical chunk boundaries vs. the old
+  linear scan.
+
+### 2. Pool the probe MemoryStream (committed)
+
+- Commit `8851fed`.
+- `GetSerializedRequestLengthForBatching` previously allocated a new `MemoryStream`
+  (and backing byte array) on **every** probe call. With galloping/binary search this can
+  still be thousands of allocations for large tag counts.
+- Now reuses a `[ThreadStatic] MemoryStream` across calls, resetting it via
+  `SetLength(0)` instead of reallocating.
+- Thread-safety rationale: a single batching operation runs **synchronously on one
+  thread**, and the helper is never called reentrantly, so `[ThreadStatic]` is safe
+  without locking. The session is never used concurrently for read and write.
+
+### 3. Optimistic fast path in the batching check (implemented)
+
+- Adds an optimistic path at the top of `AppendItemsWithinPayloadLimit`: add **all**
+  candidate items of a chunk at once and check the payload limit **once**. If it fits,
+  return immediately.
+- Rationale: the PLC-negotiated item limit (`TagsPerReadRequestMax` /
+  `TagsPerWriteRequestMax`) already usually fits the negotiated PDU payload, so the whole
+  chunk typically fits in a single frame.
+- Effect: reduces full-request serializations per chunk from **O(log n)** (item 1) to
+  **1** in the common case. When the optimistic attempt overflows the limit, the code
+  rolls back and falls back to the galloping/binary-search path from item 1, so behavior
+  stays identical.
+- Covered by the existing split-point regression tests (which exercise the fallback).
+
+### 4. Conditionally skip the payload check
+
+Implemented as part of item 3: because the payload check is only performed when it can
+actually bind (optimistic attempt fails), the expensive search path is skipped entirely
+in the common case.
+
+## Verification
+
+- Build clean (0 warnings / 0 errors) for **net48** and **net8.0**.
+- Full test suite: **468/468 passing** after each step, including the batch-split
+  regression tests in `S7CommPlusProtocolSessionReceiveTests`.
+- Debug hooks used to drive the split logic from tests: e.g.
+  `DebugCreateWriteRequestBatchForTests` in `S7CommPlusProtocolSession.cs`.
+
+## Remaining backlog
+
+5. **Typed values instead of `object`.** The read path returns `List<object>`, which
+   boxes value types. Switching to typed values would cut allocations further but is an
+   **API break** and was deferred.
+
+6. **Pipelining / parallel sessions.** Issuing multiple requests concurrently could
+   overlap I/O across the 2100 round-trips. Likely **not feasible** given protocol
+   constraints: fragmented S7CommPlus response bodies do not carry the request sequence
+   number, so concurrent fragment-producing requests make fragment ownership ambiguous
+   (see `online-multiplexing-todo.md`). One foreground request stays in flight per
+   physical connection.
+
+## Behavior preserved (invariants)
+
+- Hard-abort-on-error semantics unchanged; partial results still discarded at the client
+  layer via `ThrowIfError`.
+- Chunk boundaries byte-identical to the original linear scan.
+- First item of a chunk always included unconditionally.
+- Serialized-length monotonicity is the core assumption behind the search-based
+  optimizations; if item serialization ever stopped being monotonic, the galloping/binary
+  search would need to be revisited.

src/S7CommPlusDriver.Tests/PlcTagMultiDimensionalArrayTests.cs

@@ -0,0 +1,268 @@
+using System;
+using System.Linq;
+using S7CommPlusDriver.ClientApi;
+using Xunit;
+
+namespace S7CommPlusDriver.Tests
+{
+    /// <summary>
+    /// Verifies that a multidimensional primitive PLC array (e.g. <c>Array[1..2, 1..3] of Int</c>) is reshaped into a genuine
+    /// multidimensional CLR array on read, and correctly distributed back into its individual element tags on write, instead of
+    /// collapsing into (or requiring) a flat one-dimensional array.
+    /// </summary>
+    /// <remarks>
+    /// Regression test for a design gap ported from the upstream fork's <c>e91d8f2</c> ("Extended DataTypes") commit, which
+    /// introduced dedicated <c>PlcTagXxxArrayMDim</c> types there. This driver instead extends the existing
+    /// aggregate-element mechanism (<see cref="PlcTag.SetAggregateElements"/>, <see cref="PlcTag.CompleteAggregateRead"/>,
+    /// <see cref="PlcTag.SetValue"/>) to reshape/distribute values without introducing new tag types.
+    /// </remarks>
+    public sealed class PlcTagMultiDimensionalArrayTests
+    {
+        private static VarInfo CreateTwoByThreeIntArrayVarInfo()
+        {
+            return new VarInfo
+            {
+                Name = "DB.Matrix",
+                AccessSequence = "8A0E0001.F",
+                SymbolCrc = 0x11223344,
+                Softdatatype = Softdatatype.S7COMMP_SOFTDATATYPE_INT,
+                ArrayElementCount = 6,
+                ArrayDimensions = new[]
+                {
+                    new S7CommPlusArrayDimension(1, 2),
+                    new S7CommPlusArrayDimension(1, 3),
+                },
+            };
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagReshapesMultiDimensionalReadIntoGenuineMultiDimensionalArray()
+        {
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByThreeIntArrayVarInfo());
+
+            Assert.Equal(6, tag.AggregateElements.Count);
+            for (var index = 0; index < tag.AggregateElements.Count; index++)
+            {
+                tag.AggregateElements[index].ProcessReadResult(new ValueInt((short)(index + 1)), 0);
+            }
+            tag.CompleteAggregateRead(0);
+
+            var value = Assert.IsType<short[,]>(tag.GetValue());
+            Assert.Equal(2, value.GetLength(0));
+            Assert.Equal(3, value.GetLength(1));
+            // Row-major: outermost (first declared) dimension varies slowest.
+            Assert.Equal(1, value[0, 0]);
+            Assert.Equal(2, value[0, 1]);
+            Assert.Equal(3, value[0, 2]);
+            Assert.Equal(4, value[1, 0]);
+            Assert.Equal(5, value[1, 1]);
+            Assert.Equal(6, value[1, 2]);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagDistributesMultiDimensionalWriteIntoElementTags()
+        {
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByThreeIntArrayVarInfo());
+
+            var writeValue = new short[2, 3] { { 10, 20, 30 }, { 40, 50, 60 } };
+            tag.SetValue(writeValue);
+
+            var elementValues = tag.AggregateElements
+                .Select(element => ((PlcTagInt)element).Value)
+                .ToArray();
+            Assert.Equal(new short[] { 10, 20, 30, 40, 50, 60 }, elementValues);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagKeepsFlatArrayForOneDimensionalAggregate()
+        {
+            var varInfo = new VarInfo
+            {
+                Name = "DB.Values",
+                AccessSequence = "8A0E0001.F",
+                SymbolCrc = 0x55667788,
+                Softdatatype = Softdatatype.S7COMMP_SOFTDATATYPE_INT,
+                ArrayElementCount = 3,
+                ArrayDimensions = new[] { new S7CommPlusArrayDimension(0, 3) },
+            };
+
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo);
+            for (var index = 0; index < tag.AggregateElements.Count; index++)
+            {
+                tag.AggregateElements[index].ProcessReadResult(new ValueInt((short)(index + 1)), 0);
+            }
+            tag.CompleteAggregateRead(0);
+
+            var value = Assert.IsType<short[]>(tag.GetValue());
+            Assert.Equal(new short[] { 1, 2, 3 }, value);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagReshapesMultiDimensionalBoolReadIntoGenuineMultiDimensionalArray()
+        {
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_BOOL, "DB.Flags", 0x22334455));
+            var expected = new[] { true, false, false, true };
+            for (var index = 0; index < tag.AggregateElements.Count; index++)
+            {
+                tag.AggregateElements[index].ProcessReadResult(new ValueBool(expected[index]), 0);
+            }
+            tag.CompleteAggregateRead(0);
+
+            var value = Assert.IsType<bool[,]>(tag.GetValue());
+            Assert.Equal(2, value.GetLength(0));
+            Assert.Equal(2, value.GetLength(1));
+            Assert.True(value[0, 0]);
+            Assert.False(value[0, 1]);
+            Assert.False(value[1, 0]);
+            Assert.True(value[1, 1]);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagDistributesMultiDimensionalBoolWriteIntoElementTags()
+        {
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_BOOL, "DB.Flags", 0x22334455));
+            tag.SetValue(new[,] { { true, false }, { false, true } });
+
+            var elementValues = tag.AggregateElements
+                .Select(element => ((PlcTagBool)element).Value)
+                .ToArray();
+            Assert.Equal(new[] { true, false, false, true }, elementValues);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagReshapesMultiDimensionalRealReadIntoGenuineMultiDimensionalArray()
+        {
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_REAL, "DB.Measurements", 0x33445566));
+            var expected = new[] { 1.5f, 2.5f, 3.5f, 4.5f };
+            for (var index = 0; index < tag.AggregateElements.Count; index++)
+            {
+                tag.AggregateElements[index].ProcessReadResult(new ValueReal(expected[index]), 0);
+            }
+            tag.CompleteAggregateRead(0);
+
+            var value = Assert.IsType<float[,]>(tag.GetValue());
+            Assert.Equal(2, value.GetLength(0));
+            Assert.Equal(2, value.GetLength(1));
+            Assert.Equal(1.5f, value[0, 0]);
+            Assert.Equal(2.5f, value[0, 1]);
+            Assert.Equal(3.5f, value[1, 0]);
+            Assert.Equal(4.5f, value[1, 1]);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagDistributesMultiDimensionalRealWriteIntoElementTags()
+        {
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_REAL, "DB.Measurements", 0x33445566));
+            tag.SetValue(new[,] { { 1.5f, 2.5f }, { 3.5f, 4.5f } });
+
+            var elementValues = tag.AggregateElements
+                .Select(element => ((PlcTagReal)element).Value)
+                .ToArray();
+            Assert.Equal(new[] { 1.5f, 2.5f, 3.5f, 4.5f }, elementValues);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagReshapesMultiDimensionalStringReadIntoGenuineMultiDimensionalArray()
+        {
+            var varInfo = CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_STRING, "DB.Texts", 0x44556677);
+            varInfo.MaxStringLength = 10;
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo);
+            var expected = new[] { "aa", "bb", "cc", "dd" };
+            for (var index = 0; index < tag.AggregateElements.Count; index++)
+            {
+                tag.AggregateElements[index].ProcessReadResult(CreateStringReadValue(expected[index], 10), 0);
+            }
+            tag.CompleteAggregateRead(0);
+
+            var value = Assert.IsType<string[,]>(tag.GetValue());
+            Assert.Equal(2, value.GetLength(0));
+            Assert.Equal(2, value.GetLength(1));
+            Assert.Equal("aa", value[0, 0]);
+            Assert.Equal("bb", value[0, 1]);
+            Assert.Equal("cc", value[1, 0]);
+            Assert.Equal("dd", value[1, 1]);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagDistributesMultiDimensionalStringWriteIntoElementTags()
+        {
+            var varInfo = CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_STRING, "DB.Texts", 0x44556677);
+            varInfo.MaxStringLength = 10;
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo);
+            tag.SetValue(new[,] { { "aa", "bb" }, { "cc", "dd" } });
+
+            var elementValues = tag.AggregateElements
+                .Select(element => ((PlcTagString)element).Value)
+                .ToArray();
+            Assert.Equal(new[] { "aa", "bb", "cc", "dd" }, elementValues);
+        }
+
+        [Fact]
+        public void CreateResolvedPlcTagReshapesMultiDimensionalDtlStructReadIntoGenuineMultiDimensionalArray()
+        {
+            var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_DTL, "DB.Timestamps", 0x66778899));
+            var expected = new[]
+            {
+                new DateTime(2020, 1, 1),
+                new DateTime(2020, 1, 2),
+                new DateTime(2020, 1, 3),
+                new DateTime(2020, 1, 4),
+            };
+            for (var index = 0; index < tag.AggregateElements.Count; index++)
+            {
+                tag.AggregateElements[index].ProcessReadResult(CreateDtlReadValue(expected[index]), 0);
+            }
+            tag.CompleteAggregateRead(0);
+
+            var value = Assert.IsType<DateTime[,]>(tag.GetValue());
+            Assert.Equal(2, value.GetLength(0));
+            Assert.Equal(2, value.GetLength(1));
+            Assert.Equal(expected[0], value[0, 0]);
+            Assert.Equal(expected[1], value[0, 1]);
+            Assert.Equal(expected[2], value[1, 0]);
+            Assert.Equal(expected[3], value[1, 1]);
+        }
+
+        private static VarInfo CreateTwoByTwoVarInfo(uint softdatatype, string name, uint symbolCrc)
+        {
+            return new VarInfo
+            {
+                Name = name,
+                AccessSequence = "8A0E0001.F",
+                SymbolCrc = symbolCrc,
+                Softdatatype = softdatatype,
+                ArrayElementCount = 4,
+                ArrayDimensions = new[]
+                {
+                    new S7CommPlusArrayDimension(0, 2),
+                    new S7CommPlusArrayDimension(0, 2),
+                },
+            };
+        }
+
+        private static ValueUSIntArray CreateStringReadValue(string text, byte maxLength)
+        {
+            var bytes = System.Text.Encoding.GetEncoding("ISO-8859-1").GetBytes(text);
+            var b = new byte[maxLength + 2];
+            b[0] = maxLength;
+            b[1] = (byte)bytes.Length;
+            Array.Copy(bytes, 0, b, 2, bytes.Length);
+            return new ValueUSIntArray(b);
+        }
+
+        private static ValueStruct CreateDtlReadValue(DateTime value)
+        {
+            var structValue = new ValueStruct(0x02000043);
+            var barr = new byte[12];
+            barr[0] = (byte)(value.Year >> 8);
+            barr[1] = (byte)value.Year;
+            barr[2] = (byte)value.Month;
+            barr[3] = (byte)value.Day;
+            barr[5] = (byte)value.Hour;
+            barr[6] = (byte)value.Minute;
+            barr[7] = (byte)value.Second;
+            structValue.AddStructElement(0x02000043, new ValueByteArray(barr));
+            return structValue;
+        }
+    }
+}