Skip to content

Improve decoder throughput with buffered scalar reads - #299

Draft
Saurabh Singh (saurabh500) wants to merge 1 commit into
mainfrom
dev/saurabh/sync-buffered-reads
Draft

Improve decoder throughput with buffered scalar reads#299
Saurabh Singh (saurabh500) wants to merge 1 commit into
mainfrom
dev/saurabh/sync-buffered-reads

Conversation

@saurabh500

@saurabh500 Saurabh Singh (saurabh500) commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Description

Improves row-decoder throughput by reading buffered byte, little-endian u16, and little-endian i32 values synchronously from NetworkTransport. A buffer miss falls back to the existing async TDS packet read/refill path and retries, so packet framing and error handling remain authoritative.

This is a standalone runtime optimization informed by measurements from #269. It does not revive that PR or retry decoder convergence.

Implementation

  • Adds non-consuming-on-miss scalar probes to both TdsPacketReader configurations, with safe None defaults for other readers.
  • Implements direct probes in TdsReadBuffer and delegates them from NetworkTransport.
  • Keeps read_tds_packet() as the only refill path for async misses.
  • Routes the corresponding hot row-decoder reads through sync-first/async-fallback macros without changing unrelated decoder structure.

Correctness coverage

Targeted tests cover complete buffered reads, zero consumption when a scalar is incomplete, and u16/i32 values split across real TDS packet boundaries through NetworkTransport. Existing NULL and length-marker handling remains unchanged.

Production-reader benchmark

Compared clean main at ac1023a1 with this change using concrete NetworkTransport, packetized in-memory input, and 7,992-byte packet payloads. Each cell decoded 30,000 rows per pass with 2 warmups and 9 measured passes, across 8 paired rounds alternating baseline/candidate order. Negative deltas are faster.

Workload Sink Paired median Range Main median Candidate median
INT/VARCHAR discard -6.06% -15.86% to +11.50% 62.25 ms 58.46 ms
INT/VARCHAR materialize -14.32% -21.77% to -11.27% 99.75 ms 85.89 ms
mixed discard -11.04% -22.62% to -1.52% 128.02 ms 112.58 ms
mixed materialize -10.49% -20.84% to +3.20% 172.04 ms 153.39 ms

The INT/VARCHAR discard cell is noisy, and one mixed-materialize round regressed, but all paired medians improved and the materialized/mixed results are consistently strong. Builds used the same Cargo.lock hash and isolated target directories; the resulting binaries had different SHA-256 hashes, ruling out accidental artifact reuse.

Validation

  • cargo bfmt
  • cargo bclippy
  • Targeted nextest coverage for the new probes, packet-boundary fallback, and affected decoder tests
  • cargo nextest run -p mssql-tds --lib -E 'not test(/certificate_validator|win_tls::validate/)' (1,730 passed; fixture-dependent TLS tests excluded)
  • $env:RUSTFLAGS='--cfg fuzzing'; cargo check -p mssql-tds --lib
  • Full PR validation passed on Linux, Linux ARM64, Windows, Windows ARM, macOS, Kerberos, and cross-repo Python jobs (build 167270)
  • Coverage report passed with 100% diff coverage (144/144 changed lines) and 91.5% overall coverage
  • cargo btest was attempted locally but is not green in this environment: live SQL Server/TLS tests fail with Schannel SEC_E_WRONG_PRINCIPAL, and generated local certificate fixtures are absent. The corresponding remote validation is green.

Risk

The fast path is limited to already-buffered scalars on NetworkTransport. Other packet readers retain their existing async behavior through default None probes. Misses preserve partial bytes and reuse the existing refill path, including cancellation and encryption behavior.

Related Issues

Related to #247.

Historical measurement context: #269.

Checklist

  • cargo bfmt passes
  • cargo bclippy passes
  • cargo btest passes locally (environment-blocked; remote PR validation passes)
  • New/changed functionality has tests
  • Public API changes are documented

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Optimizes TDS row decoding by synchronously consuming buffered scalar values while retaining the existing asynchronous packet-refill fallback.

Changes:

  • Adds optional scalar probes to TdsPacketReader.
  • Implements buffered byte, u16, and i32 reads for NetworkTransport.
  • Routes decoder hot paths through sync-first reads and adds boundary tests.

Reviewed changes

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

File Description
mssql-tds/src/io/packet_reader.rs Defines non-consuming scalar probe methods.
mssql-tds/src/datatypes/decoder.rs Uses sync-first scalar reads in decoder paths.
mssql-tds/src/connection/transport/network_transport.rs Delegates probes and retains packet-refill fallback.
mssql-tds/src/connection/transport/buffers.rs Implements scalar probes and unit tests.

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

@saurabh500

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@github-actions

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

100%

🎯 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/transport/buffers.rs (100%)
  • mssql-tds/src/connection/transport/network_transport.rs (100%)
  • mssql-tds/src/datatypes/decoder.rs (100%)
  • mssql-tds/src/io/packet_reader.rs (100%)

Summary

  • Total: 144 lines
  • Missing: 0 lines
  • Coverage: 100%

🔗 Quick Links

View Azure DevOps Build · Coverage Report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants