Sans-I/O TDS core (1/N): extract PacketBuffer - #189
Closed
Saurabh Singh (saurabh500) wants to merge 1 commit into
Closed
Sans-I/O TDS core (1/N): extract PacketBuffer#189Saurabh Singh (saurabh500) wants to merge 1 commit into
Saurabh Singh (saurabh500) wants to merge 1 commit into
Conversation
Introduce PacketBuffer, the synchronous I/O-free half of packet reading: it owns the reassembled payload bytes and serves every scalar/byte read straight from memory, with the .await confined to the refill edge. Rewire PacketReader onto PacketBuffer so the buffer math and scalar decode live in one sans-I/O type; the reader only pulls bytes off the socket on refill. Public API unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-tds/src/io/packet_buffer.rsmssql-tds/src/io/packet_reader.rs🔗 Quick Links |
Saurabh Singh (saurabh500)
marked this pull request as ready for review
August 10, 2026 06:38
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 10, 2026 06:38
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Extracts synchronous packet buffering and decoding from PacketReader into a sans-I/O core.
Changes:
- Adds
PacketBufferfor buffering, decoding, copying, and skipping. - Rewires
PacketReaderto handle only asynchronous refills. - Registers the internal buffer module.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mssql-tds/src/io/packet_buffer.rs |
Implements the sans-I/O packet buffer. |
mssql-tds/src/io/packet_reader.rs |
Delegates buffered reads to PacketBuffer. |
mssql-tds/src/io.rs |
Registers the new internal module. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+161
to
+165
| pub(crate) fn begin_refill(&mut self) -> usize { | ||
| let remaining = self.available(); | ||
| if remaining > 0 { | ||
| self.working_buffer | ||
| .copy_within(self.position..self.length, 0); |
Saurabh Singh (saurabh500)
marked this pull request as draft
August 10, 2026 06:49
This was referenced Aug 10, 2026
Saurabh Singh (saurabh500)
marked this pull request as ready for review
August 10, 2026 07:39
Saurabh Singh (saurabh500)
marked this pull request as draft
August 10, 2026 13:29
This was referenced Aug 13, 2026
Closed
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.
Stack
Bottom layer of the sans-I/O
mssql-tdsrestructure. This is layer 1 of a GitHub PR stack; higher layers build on this branch.What this layer does
Introduces
PacketBuffer— the synchronous, I/O-free half of packet reading. It owns the reassembled TDS payload bytes and serves every scalar/byte read straight from memory, knowing nothing about sockets orasync. The only thing it can't do itself is obtain more bytes; when a read needs more than what's buffered, the caller (a thin I/O shell) refills viabegin_refill/commit_packetand retries.PacketReaderis rewired ontoPacketBuffer, so the buffer math and scalar decode now live in one place. The reader keeps only the one thing the buffer can't do — pull bytes off the socket on refill (receive_packet). The four ad-hoc buffer fields collapse to a singlebuffer: PacketBuffer, and the duplicatedconsume_bytes/do_we_have_enough_datalogic is gone.Public API is unchanged — this is a pure internal refactor that establishes the sans-I/O abstraction.
Why
The TDS protocol logic (framing, scalar decode) is pure computation over an in-memory byte buffer; the only real I/O is "the buffer is empty, give me more bytes." Making that split explicit lets a sync shell and an async shell later drive the same protocol core with the
.awaitconfined to a ~10-line refill loop.Next layers (preview)
PacketBufferin the productionNetworkTransportread path, deleting the duplicatedTdsReadBufferscalar/refill logic.step()core, add the blocking shell + syncTdsClientsurface, rewiremssql-odbcoffblock_on.Validation
cargo build -p mssql-tds— cleancargo bclippy— clean (-D warnings)cargo bfmt— cleancargo nextest run -p mssql-tds packet_reader— 15/15 passFraming (correctness / architecture, not a perf win yet)
This PR is the bottom of a 15-PR sans-I/O stack (#189 through #208) restructuring
mssql-tdsinto one protocol core with a sync shell and an async shell. The whole effort is a correctness/architecture refactor: it deletes the per-rowblock_ontax and thefast::*sync-fake hack, and lets sync consumers (mssql-odbc, themssql-py-coresync cursor) and async consumers (themssql-py-corecoroutine cursor) drive the same buffer-driven parser with.awaitconfined to a ~10-line refill loop.It is not yet a native-beating performance win, and nothing in this stack claims to beat
msodbcsql18. The remaining performance debt to burn down next: VarcharMax/PLP streaming (~818x on both the async-before and sync-after variants, pre-existing), the Decimal/DateTime2 conversion gaps, and the per-columnSQLGetDataconversion/alloc path.Related work item / issue: Tracked as part of the sans-I/O native stack (#192)