Sans-I/O TDS core (4c/N): invert the NBCROW null-bitmap read to sync - #197
Conversation
The only NBCROW-specific async wire read left after L4a/L4b is the fixed-width null-bitmap read at row entry. Add a read_null_bitmap seam (default via read_bytes; PacketReader/NetworkTransport override with ensure + atomic take_bytes) and route the NBCROW entry through it, so async and sync share one bitmap-read body. The whole NBCROW eager row is now sync: bitmap + non-PLP columns (L4a) + PLP collect (L4b). No new resumable machine: reuse ensure/NeedBytes and RowPauseState.nbc_null_bitmap. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761
There was a problem hiding this comment.
Pull request overview
Moves NBCROW null-bitmap reads onto the shared synchronous packet-buffer path.
Changes:
- Adds atomic
PacketBuffer::take_bytes. - Adds and forwards
read_null_bitmap. - Adds packet-boundary regression tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
token_stream.rs |
Routes NBCROW decoding through the new seam and adds tests. |
packet_reader.rs |
Defines and implements read_null_bitmap. |
packet_buffer.rs |
Adds atomic owned byte extraction. |
network_transport.rs |
Implements buffered bitmap reads for production transport. |
Suppressed comments (1)
mssql-tds/src/io/token_stream.rs:2158
- This repeats the full decode and packet-splitting harness from the test immediately above. Reuse a shared module-level helper and leave each test responsible only for its metadata, payload, and assertions; otherwise fixes to the boundary simulation must be kept synchronized manually.
async fn decode(read_data: Vec<u8>, columns: &[ColumnMetadata]) -> Vec<ColumnValues> {
let mut mock = MockNetworkReaderWriter::new(read_data, 0);
let mut reader = PacketReader::new(&mut mock);
reader.read_tds_packet_for_test().await.unwrap();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| async fn decode(read_data: Vec<u8>, columns: &[ColumnMetadata]) -> Vec<ColumnValues> { | ||
| let mut mock = MockNetworkReaderWriter::new(read_data, 0); | ||
| let mut reader = PacketReader::new(&mut mock); | ||
| reader.read_tds_packet_for_test().await.unwrap(); |
|
Closing: the sans-I/O restructuring is not going to be productionized. The row-decode performance work that motivated much of this has been re-scoped around #247, where benchmarked spikes show the dominant win comes from a far smaller change — converting This is cleanup rather than a rejection of the analysis. The branch is deliberately not deleted, so the work remains recoverable if the direction is revisited. |
Summary
Sans-I/O TDS refactor stack L4c (part of stack #192, off #186). Finishes the row-decode inversion by making the NBCROW path fully sync.
After L4a (non-PLP columns sync) and L4b (PLP collect sync), the only NBCROW-specific async wire read left was the fixed-width null-bitmap read at row entry. This inverts exactly that byte pull to the shared sync
PacketBuffercore.What changed
PacketBuffer::take_bytes(n)— atomic owned take, mirrors the scalartake_*accessors.TdsPacketReader::read_null_bitmap(bitmap_len)— new seam modeled 1:1 ondecode_column_into:read_bytes(non-buffer-owning readers),PacketReaderoverride:ensure(n)+take_bytes(atomic),NetworkTransportoverride:ensure_or_refill(n)+take_bytes(atomic),Box<dyn>blanket forward.receive_row_into_internalNBCROW arm routes throughread_null_bitmap.Async and sync now share one bitmap-read body. The whole NBCROW eager row is sync by construction: bitmap (L4c) + non-PLP columns (L4a) + PLP collect (L4b).
No new machines
Zero new resumable machines. Reuses
PacketBuffer::ensure/NeedBytes{shortfall}and the existingRowPauseState.nbc_null_bitmap(bitmap read once at entry, carried across pauses, never re-read).ensurekeeps the forward-progressdebug_assert; the take is all-or-nothing so a short buffer re-drives from row entry with nothing partial copied. Byte-identical async output.Tests (buffer-driven PacketReader + two_packets split-sweep)
nbcrow_bitmap_read_resumes_byte_identical_across_refill_boundary— 9 columns force a 2-byte bitmap; boundary swept INSIDE the multi-byte bitmap; byte-identical to single-packet baseline.fully_sync_nbcrow_mixed_row_is_byte_identical_across_refill_boundary— NBCROW row mixing NULL-via-bitmap, non-PLP inverted (int4 + varchar(64)), and PLP (varbinary(max)); boundary swept across bitmap end, column transitions, inside the PLP chunk.nbcrow_pause_and_plp_resume_path_is_exercisedstays green.Validation
git merge-base --is-ancestor 149fa6f9 HEAD: exit 0.cargo nextest -p mssql-tds --lib --no-fail-fast: FAIL-set == exactly the 7 known cert fixtures (Compare-Object vs baseline EMPTY); no hang.cargo bfmtclean;cargo bclippy(-D warnings) clean;scripts/bfmt.ps1+scripts/bclippy.ps1clean;mssql-py-corebuilds.