Perf | Reduce async read memory allocations by recycling StateSnapshot packet nodes

[priyankatiwari08]

Description

This is a memory allocation performance improvement. It restores allocation behaviour in the async read path, which regressed against the 6.1.6 baseline.

Every packet appended to a snapshot during an async read needs a PacketData linked-list node. Before #3534 these nodes were reused via a _sparePacket slot. #3534 removed that reuse, so ClearPackets now discards the whole chain on every ResetSnapshot and each subsequent async read re-allocates it from scratch. The cost scales with packets per read, which is why async reader benchmarks are hit hardest while sync ones barely move.

Worth calling out where the volume comes from, since a single node is only ~40 bytes. A snapshot is taken and released on every async continuation, not once per query — SqlDataReader.PrepareAsyncInvocation calls SetSnapshot() each time an async read has to yield. The regression is therefore driven by the number of snapshots, which is large, rather than by packets per snapshot, which is usually one. That is also why the old single-slot _sparePacket was enough to keep allocation flat despite only ever holding one node: it was reused across the whole stream of continuations. ExecuteReaderAsync allocating ~12 MB extra is roughly 300k continuations over the benchmark, not 300k nodes alive at once.

This restores node reuse: Buffer/Read are mutable again (via Initialize/Reset) and StateSnapshot keeps a bounded 16-entry free list. The bound is a small generalisation of the old single slot; 16 is what stopped the multi-packet DataTypeReaderAsync benchmarks from regressing, while the three headline cases would have been fixed by a single entry.

Measurements

Validated on the internal sqlclient-perf pipeline, build 166806, which runs the full suite of 14 runners / 162 benchmarks at the default UseOptimizedAsyncBehaviour: false. Both columns below are the allocation delta against the same 6.1.6 baseline, comparing main against this branch.

10 of the 162 benchmarks are excluded because the 6.1.6 baseline itself was not reproducible between the two runs (up to 383% drift on the same baseline build, all of them low-absolute-allocation connection-pool or ReadLargeDataSync cases). Any delta computed against a moving baseline is meaningless in either direction. The 152 benchmarks below have a baseline stable to within 5%, and the ones that matter here are stable to within 0.2%.

Bucket	main	this branch
Benchmarks >= +100% allocated	3	0
Benchmarks >= +80% allocated	3	0
Benchmarks >= +50% allocated	3	0
Benchmarks >= +25% allocated	3	0
Benchmarks >= +10% allocated	14	6
Worst allocation delta	+120.9%	+15.9%

Per-benchmark, for everything that was at or above +80% on main:

Benchmark	6.1.6	main	this branch
SqlCommand/ExecuteReaderAsync	10,347,440	+120.9%	+0.1%
MarsOverhead/ExecuteReaderAsyncWithMars[MARS=False]	1,052,112	+118.7%	-0.1%
MarsOverhead/ExecuteReaderAsyncWithMars[MARS=True]	1,052,672	+118.4%	0.0%

These are the only benchmarks in the suite above +80%, and all three are async reads. Below that threshold the DataTypeReaderAsync family also comes down from +18-23% to +1-2%, and SqlConnection/OpenConnection[MARS=False; Pooling=True] from +11.2% to +1.9%.

The six entries still at or above +10% after this change are all connection-pool runners (ConnectionPoolContention, ConnectionPoolChurn, ConnectionPoolStress) sitting at +10-16%, effectively unchanged by this PR. They are a separate regression on the pooling path and are not addressed here.

Ensuring this does not reintroduce the bugs #3534 fixed

#3534 removed packet reuse while fixing several distinct problems. Reuse itself was not the defect. Quoting that PR:

The logic to clear the spare packet was faulty and did not clear all the fields leaving the data length in the node.

The pre-#3534 Clear() reset Buffer, Read, NextPacket, PrevPacket and the debug fields, but never reset RunningDataSize. A recycled node could therefore be reinstalled as the head of a new chain still carrying a stale running length, and GetPacketDataOffset/GetPacketDataSize would compute wrong values once a read reached the continue stage at 3 or more packets. Removing reuse made the stale field unreachable, which fixed the symptom.

This change addresses the actual defect instead. Both Initialize and Reset explicitly set RunningDataSize = 0, so a node cannot carry a stale length across uses regardless of which path it takes. That is the one field whose omission caused #3519.

The rest of #3534 is untouched. Its plp terminator fix, the char array sizing fix, and the TryReadColumnInternal fallthrough fix for #3572 all live elsewhere in the file; the diff here is confined to the PacketData class and StateSnapshot.

Verified by running #3534's own regression test, CanReadAwkwardDataLengths, which sweeps packet sizes from 512 to 2048 in steps of 3 and is the direct repro for #3519. It passes, as does the full DataReaderTest class.

The recycling implementation is also stricter than the pre-#3534 one it replaces: the free list is bounded at 16 entries rather than an untyped single slot, and ResetDebugState() clears DebugPacketId/Stack/Hash in DEBUG builds so a recycled node cannot carry stale debug state into the duplicate and overlap assertions.

Compatibility

No public API changes. PacketData is a private sealed class nested inside TdsParserStateObject; every reference to it lives inside StateSnapshot (_firstPacket, _lastPacket, _current, _continuePacket, _sparePackets), all of which are cleared by ClearPackets before any node is recycled. No caller outside the snapshot can observe a recycled node. Nodes never own the packet buffers they point at, so Reset() only drops this node's references and buffer lifetime is unchanged.

Issues

N/A

Testing

• New unit tests, StateSnapshotPacketRecyclingTests (5 tests, pass on net462/net8.0/net9.0 in Debug and Release). They drive 20 packets per snapshot across repeated snapshot/clear cycles, which is above the 16-entry bound, and assert:

  • a recycled node reports RunningDataSize == 0, and the offsets/lengths derived from it are correct — the direct guard for 6.1.0: Errors while executing the query #3519;
  • a parked node holds no buffer, length, running total or chain link;
  • the free list respects its bound and its tracked count matches its real length;
  • no node is ever reachable from both the live chain and the free list;
  • the rebuilt chain has intact NextPacket/PrevPacket links in both directions.

  Confirmed meaningful by reverting the RunningDataSize = 0 lines in Initialize/Reset: two of the five fail, including the 6.1.0: Errors while executing the query #3519 guard.

• Internal sqlclient-perf pipeline run of this branch against the 6.1.6 baseline, full suite: build 166806. Results above.

• CanReadAwkwardDataLengths passes. This is the regression test added by Async multi packet fixes #3534 for 6.1.0: Errors while executing the query #3519 and is the most direct check that this change does not reintroduce that bug.

• Full DataReaderTest class: 17 passed, 0 failed.

• Functional test suite: 1347 passed. The 5 failures are AlwaysEncrypted certificate-store tests that require administrator rights and are unrelated to this change.

• Local perf suite run three times (6.1.6, main, this branch) at the default UseOptimizedAsyncBehaviour: false, agreeing with the pipeline.

Guidelines

Please review the contribution guidelines before submitting a pull request:

• Contributing
• Code of Conduct
• Best Practices
• Coding Style
• Review Process

[Copilot]

Pull request overview

Fixes a memory regression in the packet multiplexer by avoiding per-read 8 KB buffer reallocation unless the active read buffer is actually retained by a StateSnapshot, restoring the allocation profile of the legacy compat path while keeping snapshot replay correctness.

Changes:

• Introduces _inBuffRetained tracking in TdsParserStateObject and sets it at concrete snapshot retention points.
• Updates ProcessSniPacket to reallocate the read buffer only when _inBuffRetained is true (and _inBytesRead != 0), preventing unnecessary allocations.
• Adds functional tests (plus test harness plumbing) to validate buffer reuse vs. snapshot-retained replacement behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs	Adds _inBuffRetained state and sets it in snapshot capture/replay paths to reflect real buffer ownership.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs	Uses _inBuffRetained to gate buffer reallocation in ProcessSniPacket; marks retention when snapshot appends reference data.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/TdsParserStateObject.TestHarness.cs	Mirrors new members in the functional test harness stub so multiplexer code can compile/run in the test project.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/MultiplexerTests.cs	Adds tests verifying buffer reuse when unretained and replacement when snapshot retains the buffer.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.97%. Comparing base (ee529d4) to head (7a9b083).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4536      +/-   ##
==========================================
- Coverage   64.78%   62.97%   -1.81%     
==========================================
  Files         288      283       -5     
  Lines       44418    67450   +23032     
==========================================
+ Hits        28774    42477   +13703     
- Misses      15644    24973    +9329     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	62.97% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs:4880

• Recycled PacketData nodes have Buffer reset to null, but the DEBUG-only header helpers a few lines above (SPID/IsEOM/DataLength/GetHeaderSpan) unconditionally slice Buffer. This can throw during debugger inspection of the free-list nodes (especially now that ResetDebugStateImpl makes it more likely these nodes show up as 'empty'). Guard those helpers so free-list nodes are safe to inspect.

                partial void SetDebugPacketIdImpl(int value) => DebugPacketId = value;

                partial void ResetDebugStateImpl()

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs:1

• The file now starts with a UTF-8 BOM (\uFEFF). Other C# files in this repo appear not to use BOMs (e.g., TdsParser.cs, SqlConnection.cs), so this introduces unnecessary encoding churn in diffs/blame.

// Licensed to the .NET Foundation under one or more agreements.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs:4703

• PacketData.Reset() can set Buffer to null when a node is returned to the free list, but PacketID unconditionally dereferences Buffer. This can cause NullReferenceException during debugger evaluation (DebuggerDisplay/ToString) or if a freed node is accidentally inspected/logged. Making PacketID null/length-safe keeps recycled nodes inert.

This issue also appears on line 4878 of the same file.

                public int PacketID => Packet.GetIDFromHeader(Buffer.AsSpan(0, TdsEnums.HEADER_LEN));

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[cheenamalhotra]

Verdict: Approve with minor comments. The change is correct, well-scoped (one file, one private nested type), and the safety argument holds up under verification.

What I verified independently

1. The historical claim is accurate. git show 2acf6e005 (#3534) confirms it removed a _sparePacket slot whose PacketData.Clear() reset Buffer, Read, NextPacket, PrevPacket and the debug fields but not RunningDataSize — exactly the defect behind #3519. Both Initialize and Reset here set it to 0, so the actual bug is fixed rather than made unreachable.

2. Builds clean in Debug (net9.0), which matters here since ResetDebugStateImpl only exists under #if DEBUG.

3. Free-list invariants hold. I ran a reflection harness against the Release assembly driving AppendPacketData/ClearPackets over 5 rounds (20 packets, then 5x5) and asserted:

   • no duplicate nodes on the free list, and no node simultaneously on the free list and the live chain (the aliasing failure that would cause silent data corruption);
   • RunningDataSize, Buffer, Read, PrevPacket all cleared on parked nodes;
   • _sparePacketCount matches the actual list length, and the cap of 16 is respected across repeated cycles;
   • the rebuilt chain has intact Prev/Next links and a correct _lastPacket.

   All checks passed.

4. No live reference can be recycled. Every PacketData reference (_firstPacket, _lastPacket, _current, _continuePacket) is nulled inside ClearPackets before any node is parked, and MoveNext/MoveToContinue copy Buffer out via SetBuffer rather than retaining nodes. RentPacket also correctly reads NextPacket before Initialize nulls it.

The causal story is worth stating explicitly

Pre-#3534 reuse only ever recycled one node (_firstPacket), since ClearPackets parked just the head. So the +120% regression comes from the sheer number of snapshots — one per async continuation, mostly single-packet — not from packets-per-snapshot. That is consistent with your measurements and worth a sentence in the description; it pre-empts the obvious "how can one node account for 12 MB?" question during review.

Corollary: the three headline benchmarks would be fixed by a 1-entry list; the 16-entry list is what buys the DataTypeReaderAsync improvement. Please justify 16 in the comment (or note that it is empirical), since it currently reads as a bare magic number.

Minor comments

• CaptureAsStart silently gains _continuePacket = null. Replacing the three assignments with ClearPackets() also clears _continuePacket, which the old code left alone. It is a no-op today (every caller path runs Clear() first) and defensively correct, but it is an unadvertised semantic change in the diff — worth calling out.
• Asymmetric API. RentPacket has no matching ReturnPacket; the park logic is inlined in ClearPackets. Extracting ReturnPacket(PacketData) would put the bound and the Reset() ordering in one auditable place.
• Constructor duplication. PacketData(byte[], int) should delegate to Initialize(buffer, read) so the two initialization paths cannot drift. That drift is precisely how the RunningDataSize omission survived last time.
• No automated test. Repo guidance asks for one. I could not run CanReadAwkwardDataLengths locally (no SQL Server configured), and the real StateSnapshot is not reachable from FunctionalTests — TdsParserStateObject.TestHarness.cs stubs it out with its own list-based type. Consider either widening the harness to exercise the real snapshot, or adding a manual test that drives more than 16 packets across repeated async reads with continue enabled, which is the one shape existing coverage does not hit.
• Retention is roughly 16 x ~56 B = ~900 B per TdsParserStateObject, held via _cachedSnapshot for the connection's lifetime. Negligible, and the buffers themselves are nulled — no change needed, just confirming it was considered.

[priyankatiwari08]

Thanks for the review @cheenamalhotra. All six points are addressed in 50f1408.

1. Why one node accounts for that much allocation — added to the description. A snapshot is taken per async continuation, so the cost scales with the number of continuations, not packets per snapshot. That is also why the old single-slot _sparePacket was enough.
2. The 16 bound — now documented as empirical, ~900 bytes retained per state object. You are right that a single entry fixes the three headline benchmarks; 16 is what keeps the multi-packet DataTypeReaderAsync cases flat.
3. _continuePacket in CaptureAsStart — commented in place.
4. ReturnPacket — extracted to mirror RentPacket; behaviourally identical to the old loop.
5. Constructor delegates to Initialize — done. Single init path, which is what prevents a repeat of 6.1.0: Errors while executing the query #3519.
6. Tests — added StateSnapshotPacketRecyclingTests (5 tests, net462/net8.0/net9.0, Debug + Release). They drive 20 packets per snapshot, above the bound, and cover the RunningDataSize guard for 6.1.0: Errors while executing the query #3519, full state release on parked nodes, the bound and its count, that no node is shared between the live chain and the free list, and chain link integrity. Reverting the RunningDataSize = 0 lines fails 2 of the 5, so they are not vacuous. Reflection is needed because the nodes are private; the FunctionalTests harness substitutes its own StateSnapshot, so the real free list is unreachable there.

Full UnitTests suite: 1072 passed, 0 failed.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/StateSnapshotPacketRecyclingTests.cs:27

• The comment says MaxSparePacketCount must exceed StateSnapshot.MaxSparePacketCount, but the value is intentionally set to match it (16). As written, this is misleading; it’s PacketsPerSnapshot (20) that exceeds the bound to exercise the free-list cap.

        // Must exceed StateSnapshot.MaxSparePacketCount so the bound itself is exercised.
        private const int MaxSparePacketCount = 16;
        private const int PacketsPerSnapshot = 20;