Heap-allocate the PLP drain scratch buffer

[Saurabh Singh (saurabh500)]

Description

drain_active_plp held an 8 KiB scratch array on the stack across an .await, so rustc stored it inline in the generated future and that size propagated into every caller in the await chain. The row-fetch hot path therefore paid to construct and move ~8.5 KB of state per row for a cleanup path that only runs when a caller abandons a partially read PLP column.

The fix is one line — [0u8; 8192] → vec![0u8; 8192] — plus a doc comment on drain_active_plp recording why the buffer is heap-allocated, so it doesn't get "optimized" back into a stack array later.

#225 has the full explanation, the standalone criterion repro, and the measured numbers. Please read it there rather than expecting the analysis restated here.

Measured impact

Per-row future size and cost, from the standalone repro in the issue (5000 rows per sample, drain branch never taken):

	before	after
per-row future	8248 B	80 B
per-row cost	136.1 ns	15.8 ns (8.6×)

Measured in the driver itself: next_row_cursor 8520 B → 928 B, read_row_column 8464 B → 432 B. Fetch cost fell 20% on the ODBC path and 33% on the TDS column path.

The change is entirely inside mssql-tds, so mssql-js, mssql-py-core, mssql-tds-cli and mssql-odbc all benefit. No API change, no growth in steady-state memory.

Regression guard

The issue floated a size_of_val assertion to keep this from silently regressing. It came out clean, so it's included: row_fetch_futures_stay_small builds each of the four hot-path futures via the existing create_test_client() helper and asserts each stays under 4096 B. Constructing an async fn's future runs none of its body, so the futures are built and dropped unpolled — no I/O, no mock traffic.

Measured sizes under the test profile are 928 / 408 / 1208 / 960 B, giving ~3.4× headroom while still catching an 8 KiB regression. Verified it works by temporarily reverting the one-liner: the test fails with next_row_cursor future is 8480 B, expected <= 4096 B.

Doc correction

The public doc on next_row_cursor promised an "allocation-free" drain. Copilot flagged that the PLP scratch buffer invalidates it; David Engel (@David-Engel) then pointed out the replacement wording was still wrong, because skipping any non-PLP column already materializes and discards its value (see the TODO(#47154) in token_stream.rs). The doc now describes observable behavior only and makes no allocation claim.

Scope

• This is the standalone extraction of a one-line change that also appears inside the much larger in-flight perf PR Make the ODBC fetch path 2.3-3.9x cheaper on CPU (beats msodbcsql18 on varchar(max)/nvarchar) #215, bundled there with many unrelated optimizations. Split out so it can land independently. No coordination with or rebase onto Make the ODBC fetch path 2.3-3.9x cheaper on CPU (beats msodbcsql18 on varchar(max)/nvarchar) #215 is intended.
• mssql-tds/src/connection/transport/win_tls/stream.rs:387 has an identical-looking [0u8; 8192] and is deliberately left alone — it sits in a synchronous poll_read returning Poll with no await spanning it, so it is an ordinary stack local and is correct as written. Perf: 8 KiB stack scratch in drain_active_plp inflates every row-fetch future to ~8.5 KB (8.6x per-row cost) #225 calls this out explicitly; drain_active_plp was the only live instance.

Related Issues

Fixes #225

Checklist

• cargo bfmt passes
• cargo bclippy passes
• cargo btest passes — see note below
• New/changed functionality has tests
• Public API changes are documented — no API change; stale next_row_cursor doc corrected

Validation was run via .\scripts\bfmt.ps1 and .\scripts\bclippy.ps1 (both also cover mssql-py-core, which is excluded from the workspace) and both are green.

Tests were scoped with cargo nextest run --workspace --lib --no-fail-fast because integration tests require a .env that isn't present locally. Result: 2216 passed, 7 failed — the 7 are the pre-existing expired-certificate fixture tests in certificate_validator and win_tls::validate. Confirmed identical failures on a stashed, unmodified tree, so they are unrelated to this change.

[Copilot]

Pull request overview

Heap-allocates the PLP drain buffer to reduce row-fetch future sizes and hot-path overhead.

Changes:

• Replaces the 8 KiB stack buffer with a Vec.
• Documents the allocation rationale.
• Adds regression tests for future sizes.

---

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

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 55%

🎯 Overall Coverage 91.5%

📦 Project: mssql-tds + mssql-odbc + mssql-py-core ℹ️ Note: diff coverage is reported, not enforced.

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql-tds/src/connection/tds_client.rs (55.6%): Missing lines 4293-4295,4304-4306,4311,4313

Summary

• Total: 18 lines
• Missing: 8 lines
• Coverage: 55%

mssql-tds/src/connection/tds_client.rs

  4289     }
  4290 
  4291     /// Guards the fix for #225: a large local held across an `.await` in
  4292     /// `drain_active_plp` is stored inline in that future and propagates into
! 4293     /// every caller in the await chain, costing a memcpy per row on the hot path.
! 4294     #[test]
! 4295     fn row_fetch_futures_stay_small() {
  4296         const MAX: usize = 4096;
  4297 
  4298         let mut client = create_test_client();
  4299         let mut sink = DiscardRowWriter;

---

  4300 
  4301         // Constructing an async fn's future runs none of its body, so these are
  4302         // free to build and drop unpolled. Each borrow ends with its statement.
  4303         let next_row_cursor = std::mem::size_of_val(&client.next_row_cursor());
! 4304         let read_row_column = std::mem::size_of_val(&client.read_row_column(0));
! 4305         let drain_rows = std::mem::size_of_val(&client.drain_rows());
! 4306         let get_next_row_into = std::mem::size_of_val(&client.get_next_row_into(&mut sink));
  4307 
  4308         for (name, size) in [
  4309             ("next_row_cursor", next_row_cursor),
  4310             ("read_row_column", read_row_column),
! 4311             ("drain_rows", drain_rows),
  4312             ("get_next_row_into", get_next_row_into),
! 4313         ] {
  4314             assert!(
  4315                 size <= MAX,
  4316                 "{name} future is {size} B, expected <= {MAX} B"
  4317             );

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[David Engel (David-Engel)]

Summary

One-line fix ([0u8; 8192] -> vec![0u8; 8192]) that removes an 8 KiB stack array held across an .await, plus a doc update and a future-size regression guard. The change is correct and well-scoped, and the claimed numbers reproduce exactly. No blocking issues; one doc-accuracy suggestion inline.

Verification I ran locally (worktree at aa59253)

check	result
row_fetch_futures_stay_small	passes; measured 928 / 408 / 1208 / 960 B — matches the PR body exactly
guard actually catches the regression	reverted the one-liner to [0u8; 8192] -> fails with next_row_cursor future is 8480 B, expected <= 4096 B
scope claim about win_tls/stream.rs:387	correct — it is a stack local in a synchronous poll_read returning Poll, no await spans it
other large stack arrays held across awaits in mssql-tds / mssql-odbc	none found; drain_active_plp was the only live instance
CI	all checks green (ADO validation across Windows/Linux/macOS/ARM, coverage, CodeQL, Kerberos, cross-repo mssql-python)

The private rationale comment on drain_active_plp is the right call — it records exactly the thing a future reader would otherwise "optimize" back into a stack array. Keep it as written.

Blocking

None.

Suggestion

One inline comment on the next_row_cursor doc change.