Repository navigation
Feature/optimize 05 perf batching payload check - #15
Merged
jogibear9988 merged 8 commits intoOct 7, 2026
Merged
jogibear9988 merged 8 commits into
jogibear9988 merged 8 commits into
Conversation
…r lists (GML-1187)
Port net functional changes from fork package 01 (DataTypes extension / string array fix, commits 6aef271/519cdeb/ffe8daa): - PlcTag: add virtual GetValue()/SetValue(object) reflecting the concrete tag's public Value property, for generic tag value access. - POffsetInfoType: add virtual MaxStringLength(), returning 0 by default, UnspecifiedOffsetinfo1 for Array1Dim (string array max length), and Min(UnspecifiedOffsetinfo1, UnspecifiedOffsetinfo2) for String (fixes cases where firmware swaps both fields). All other changes from this fork package are already superseded by the target repo's own PlcTag/PlcTags implementation and are documented as excluded in package 16.
…ING/WSTRING writes to fail
…ltidimensional CLR arrays
ExceedsSingleFramePayload previously reserialized the entire batch request from scratch on every single item added while building a chunk in ReadValues and CreateWriteRequestBatch (used by WriteValues). For large tag counts (e.g. 210k tags at 100 tags/request) this caused ~n^2 serializations per chunk and a large number of MemoryStream allocations. Introduces AppendItemsWithinPayloadLimit, which uses a galloping search followed by a binary search to find the exact same maximum item count as the previous linear scan (the serialized length grows monotonically with the number of appended items), reducing full-request serializations per chunk from O(n) to O(log n). Behavior is unchanged: same chunk contents, same first-item-always-included semantics, same error handling on communication failures (hard abort, no partial-result handling changed). Adds a theory-based regression test asserting the batch split point matches exactly across several sizes, including power-of-two boundaries where the galloping search transitions to its binary-search phase.
…atching GetSerializedRequestLengthForBatching is called repeatedly per chunk by the galloping/binary-search batching introduced previously (potentially thousands of times for large tag counts). It previously allocated a new MemoryStream (and backing byte array) on every call just to measure the serialized request length. Reuses a [ThreadStatic] MemoryStream across calls, resetting its length instead of reallocating. Safe without locking because a single batching operation runs synchronously on one thread and this helper is never called reentrantly. Reduces GC pressure for large batched read/write operations without changing any observable behavior.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved metadata loss, array synchronization failures, incomplete TLS locking, and payload-bound performance regressions prevent approval.
Review effort: Balanced
Findings: 1
Open (8)
What changed in this PR
This PR optimizes request batching and extends tag metadata and multidimensional-array handling.
Changes:
- Replaces repeated linear payload checks with search-based batching and reusable probe buffers.
- Adds string-capacity propagation, catalog persistence, and multidimensional value handling.
- Adds TLS queue locks, regression tests, and performance documentation.
| File | Description |
|---|---|
| src/S7CommPlusDriver/S7CommPlusTagAccessorCatalog.cs | Persists string capacities in catalog format v3. |
| src/S7CommPlusDriver/S7CommPlusProtocolSession.cs | Optimizes batching and propagates tag metadata. |
| src/S7CommPlusDriver/S7CommPlusClient.cs | Adds a diagnostics import. |
| src/S7CommPlusDriver/OpenSSL/OpenSSLConnector.cs | Locks pending queue operations. |
| src/S7CommPlusDriver/Core/POffsetInfoType.cs | Exposes string-capacity metadata. |
| src/S7CommPlusDriver/ClientApi/PlcTags.cs | Applies string capacities during tag creation. |
| src/S7CommPlusDriver/ClientApi/PlcTag.cs | Adds multidimensional value access and reshaping. |
| src/S7CommPlusDriver/ClientApi/Browser.cs | Carries string capacities in browse results. |
| src/S7CommPlusDriver.Tests/S7CommPlusProtocolSessionReceiveTests.cs | Tests batching split boundaries. |
| src/S7CommPlusDriver.Tests/PlcTagStringMaxLengthTests.cs | Tests string-capacity forwarding. |
| src/S7CommPlusDriver.Tests/PlcTagMultiDimensionalArrayTests.cs | Tests multidimensional reads and assignments. |
| docs/batch-read-write-performance-optimization.md | Documents batching optimizations and constraints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+36
to
+37
| private readonly object m_readListLock = new object(); | ||
| private readonly object m_writeListLock = new object(); |
Comment on lines
+83
to
+85
| if (m_AggregateDimensions.Count > 1) | ||
| { | ||
| return AggregateValue; |
Comment on lines
+102
to
+104
| DistributeMultiDimensionalValue(multiDimValue); | ||
| AggregateValue = multiDimValue; | ||
| return; |
Comment on lines
+185
to
+187
| if (m_AggregateDimensions.Count > 1) | ||
| { | ||
| AggregateValue = ReshapeToMultiDimensional(elementValueProperty.PropertyType, flatValues, m_AggregateDimensions); |
| internal virtual void PrepareAggregateWrite() | ||
| { | ||
| if (m_AggregateElements.Count == 0) | ||
| if (m_AggregateElements.Count == 0 || m_AggregateDimensions.Count > 1) |
| /// Gets the maximum string length for a String/WString offset info, or 0 when not applicable. | ||
| /// </summary> | ||
| /// <returns>The maximum length in characters, or 0 for non-string offset info types.</returns> | ||
| public virtual int MaxStringLength() { return 0; } |
Comment on lines
+720
to
+724
| for (var i = 0; i < maxCandidateCount; i++) | ||
| { | ||
| addItemAt(i); | ||
| } | ||
| if (!ExceedsSingleFramePayload(request, maxPayloadSize)) |
Comment on lines
+368
to
+369
| tag.AggregateElements.Select(FromTag).ToArray(), | ||
| tag.GetMaxStringLength()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contains the sources from branch feature/optimize-04