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.

This restores node reuse: Buffer/Read are mutable again (via Initialize/Reset) and StateSnapshot keeps a bounded 16-entry free list.

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

• 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.