Remove per-row dispatch overhead from the row-decode path - #264
Conversation
The trait has a generic method, so it was already dyn-incompatible and the per-call boxing bought nothing. Removal exposed two latent constraints: - SQL_VARIANT decoding is genuinely recursive (decode -> read_sql_variant -> decode_zero_propbyte_variant -> decode). async_trait's box was silently breaking that cycle; a native async fn yields E0733. Reintroduced deliberately via decode_boxed, which pays one allocation per nested variant column instead of one per column read. - Callers wrap decode in async_trait futures that require Send, and the bare AFIT desugaring does not promise it, so the trait declares -> impl Future<Output = ...> + Send. Measured at roughly 0% on its own; it ships for API hygiene and as the prerequisite for making the decode chain generic over the row writer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41e46220-ad76-4a77-b477-e768951dcf92
Replaces #[async_trait] on TdsPacketReader with explicit -> impl Future<Output = ...> + Send. Every column read previously allocated a Pin<Box<dyn Future>>; a 39 INT + 9 VARCHAR row issues ~96 reads, so the boxing dominated the decode path. Two consequences the trait definition forced: - The bare AFIT desugaring does not promise Send, and callers wrap these futures in async_trait futures that do. Declaring + Send in the trait keeps every caller compiling unchanged. - RPITIT makes the trait dyn-incompatible. The seven &mut dyn parameters in PlpChunkStreamReader/PlpColumnStream were only ever parameters, so they became generic. The three stored Box<dyn TdsPacketReader> live in fuzz-only code and admitted just two concrete types, replaced by the FuzzPacketReader enum. The blanket impl for Box<dyn TdsPacketReader + Send + Sync> is gone. fuzz_support.rs is #[cfg(fuzzing)]-gated and is not built by a default cargo check, so this was verified with RUSTFLAGS='--cfg fuzzing' against both the lib and the fuzz targets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41e46220-ad76-4a77-b477-e768951dcf92
Adds W: RowWriter + Send + ?Sized to drive_row_columns, decode_or_decrypt_column, receive_row_into_internal and resume_row_into_internal, so a caller holding a concrete writer gets static dispatch on the ~96 writer calls a wide row makes. GenericDecoder::decode_into was already generic over W; these four were the only reason it always instantiated as a trait object. Because the bound is ?Sized, dyn RowWriter + Send still satisfies W, so every existing caller compiles unchanged and TdsTransport stays dyn-compatible. Production delta today is 0%, and the reason is structural rather than incidental: TdsClient holds Box<dyn TdsTransport>, and TdsTransport has TdsTokenStreamReader as a supertrait, so the writer's concrete type is erased at that vtable call before it ever reaches this chain. Neither a generic sibling method nor genericizing TdsTokenStreamReader can recover it -- a where Self: Sized method is uncallable on a trait object, and a bare generic method makes Box<dyn TdsTransport> illegal. This commit makes everything below that boundary ready, so the win lands with no further decode-path work once the transport indirection is removed. The blocked headroom is measured on the PR. resume_row_into_internal is included for symmetry. Its only callers pass dyn, so it adds no extra monomorphization; drop it if reviewers prefer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41e46220-ad76-4a77-b477-e768951dcf92
Benchmark: method, numbers, and reproRe-measured on this branch. Nothing here is copied from #247. MethodCriterion is not usable for this path:
Row shapes:
Reprocargo test --release -p mssql-tds --lib bench_row_decode -- --nocapture --test-threads=1Drop the harness at Raw medians (ms per 20,000-row pass, lower is better)
Per-commit#251 — remove #252 — RPITIT
−61.0% on the 39 INT + 9 VARCHAR(6) row reproduces the −61.6% reported on #247. #257 — generic The production path is
The The controlled headroom experimentSame row shape, same branch, same binary — only the writer's dispatch differs. Before #257
Three controls within ±2%, then a 20.0% gap once #257 lands. That gap is the Cumulative,
|
| Case | main | branch | delta |
|---|---|---|---|
| poc_row_39int_9varchar | 157.013 | 55.358 | −64.7% |
| poc_nbcrow_39int_9varchar | 147.002 | 47.721 | −67.5% |
| wide_strings_8x512 | 41.501 | 23.588 | −43.2% |
| contig_poc_row (dyn) | 139.141 | 43.759 | −68.5% |
| contig_wide (dyn) | 40.107 | 21.071 | −47.5% |
Caveats
- Microbenchmark against an in-memory reader: no syscalls, no TLS, no network. It isolates
decode CPU cost, which is the thing these commits change, but a real query's end-to-end
win will be smaller in proportion to however much time it spends in I/O. - Single machine, single OS (Windows), single rustc (1.95.0).
- Absolute values are not portable; the deltas are the point.
mononumbers for Investigate removing&mut dyn RowWriterfrom the per-column path (row decode perf, axis: dispatch) #257 are not reachable by any current caller. Do not quote them as
shipped wins.
Harness source — drop at mssql-tds/src/decode_bench.rs (670 lines)
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.
//! Throwaway row-decode microbenchmark used to measure the E1–E6 feasibility
//! spikes against their merge-base. Lives inside the crate (rather than in
//! `benches/`) because the row decode entry point and `TdsPacketReader` are
//! `pub(crate)` and a Criterion bench is a separate crate.
//!
//! Run with:
//! ```text
//! cargo nextest run --release -p mssql-tds --lib decode_bench --no-capture
//! ```
//!
//! This file is intentionally identical between the baseline worktree and the
//! feasibility branch except for the `#[async_trait]` attribute on the reader
//! impl, which the trait's own shape forces.
use std::hint::black_box;
use std::sync::Arc;
use std::time::{Duration, Instant};
use crate::core::TdsResult;
use crate::datatypes::column_values::{
ColumnValues, SqlDate, SqlDateTime, SqlDateTime2, SqlDateTimeOffset, SqlMoney,
SqlSmallDateTime, SqlSmallMoney, SqlTime, SqlXml,
};
use crate::datatypes::decoder::DecimalParts;
use crate::datatypes::row_writer::{DefaultRowWriter, RowWriter};
use crate::datatypes::sql_json::SqlJson;
use crate::datatypes::sql_string::SqlString;
use crate::datatypes::sql_vector::SqlVector;
use uuid::Uuid;
use crate::datatypes::sqldatatypes::{TdsDataType, TypeInfo};
use crate::io::packet_reader::TdsPacketReader;
use crate::io::token_stream::{
ColumnPolicy, GenericTokenParserRegistry, ParserContext, receive_row_into_internal,
};
use crate::query::metadata::ColumnMetadata;
use crate::token::tokens::{ColMetadataToken, SqlCollation, TokenType};
/// Rows decoded per timed pass.
const ROWS: usize = 20_000;
/// Untimed passes before measurement, to settle caches and branch predictors.
const WARMUP_PASSES: usize = 3;
/// Timed passes; the reported figure is the median.
const MEASURED_PASSES: usize = 9;
// ---------------------------------------------------------------------------
// In-memory packet reader
// ---------------------------------------------------------------------------
/// Serves a pre-built byte buffer with no I/O, so a measurement reflects decode
/// cost rather than socket or syscall behavior.
struct MemReader {
data: Arc<Vec<u8>>,
pos: usize,
}
impl MemReader {
fn new(data: Arc<Vec<u8>>) -> Self {
Self { data, pos: 0 }
}
#[inline]
fn take(&mut self, n: usize) -> TdsResult<&[u8]> {
let end = self.pos + n;
if end > self.data.len() {
return Err(crate::error::Error::ProtocolError(
"unexpected end of bench buffer".to_string(),
));
}
let slice = &self.data[self.pos..end];
self.pos = end;
Ok(slice)
}
}
impl TdsPacketReader for MemReader {
async fn read_byte(&mut self) -> TdsResult<u8> {
Ok(self.take(1)?[0])
}
async fn read_int16_big_endian(&mut self) -> TdsResult<i16> {
let r = self.take(2)?;
Ok(i16::from_be_bytes([r[0], r[1]]))
}
async fn read_int32_big_endian(&mut self) -> TdsResult<i32> {
let r = self.take(4)?;
Ok(i32::from_be_bytes([r[0], r[1], r[2], r[3]]))
}
async fn read_uint40(&mut self) -> TdsResult<u64> {
let r = self.take(5)?;
Ok(u64::from(r[0])
| u64::from(r[1]) << 8
| u64::from(r[2]) << 16
| u64::from(r[3]) << 24
| u64::from(r[4]) << 32)
}
async fn read_float32(&mut self) -> TdsResult<f32> {
let r = self.take(4)?;
Ok(f32::from_le_bytes([r[0], r[1], r[2], r[3]]))
}
async fn read_float64(&mut self) -> TdsResult<f64> {
let r = self.take(8)?;
Ok(f64::from_le_bytes([
r[0], r[1], r[2], r[3], r[4], r[5], r[6], r[7],
]))
}
async fn read_int16(&mut self) -> TdsResult<i16> {
let r = self.take(2)?;
Ok(i16::from_le_bytes([r[0], r[1]]))
}
async fn read_uint16(&mut self) -> TdsResult<u16> {
let r = self.take(2)?;
Ok(u16::from_le_bytes([r[0], r[1]]))
}
async fn read_uint24(&mut self) -> TdsResult<u32> {
let r = self.take(3)?;
Ok(u32::from(r[0]) | u32::from(r[1]) << 8 | u32::from(r[2]) << 16)
}
async fn read_int32(&mut self) -> TdsResult<i32> {
let r = self.take(4)?;
Ok(i32::from_le_bytes([r[0], r[1], r[2], r[3]]))
}
async fn read_uint32(&mut self) -> TdsResult<u32> {
let r = self.take(4)?;
Ok(u32::from_le_bytes([r[0], r[1], r[2], r[3]]))
}
async fn read_int64(&mut self) -> TdsResult<i64> {
let r = self.take(8)?;
Ok(i64::from_le_bytes([
r[0], r[1], r[2], r[3], r[4], r[5], r[6], r[7],
]))
}
async fn read_uint64(&mut self) -> TdsResult<u64> {
let r = self.take(8)?;
Ok(u64::from_le_bytes([
r[0], r[1], r[2], r[3], r[4], r[5], r[6], r[7],
]))
}
async fn read_bytes(&mut self, buffer: &mut [u8]) -> TdsResult<usize> {
let n = buffer.len();
buffer.copy_from_slice(self.take(n)?);
Ok(n)
}
async fn read_u8_varbyte(&mut self) -> TdsResult<Vec<u8>> {
let n = self.take(1)?[0] as usize;
Ok(self.take(n)?.to_vec())
}
async fn read_u16_varbyte(&mut self) -> TdsResult<Vec<u8>> {
let r = self.take(2)?;
let n = u16::from_le_bytes([r[0], r[1]]) as usize;
Ok(self.take(n)?.to_vec())
}
async fn read_varchar_u16_length(&mut self) -> TdsResult<Option<String>> {
let r = self.take(2)?;
let n = u16::from_le_bytes([r[0], r[1]]);
if n == crate::io::packet_reader::LENGTH_NULL {
return Ok(None);
}
Ok(Some(self.read_unicode(n as usize).await?))
}
async fn read_varchar_u8_length(&mut self) -> TdsResult<String> {
let n = self.take(1)?[0] as usize;
self.read_unicode(n).await
}
async fn read_unicode(&mut self, string_length: usize) -> TdsResult<String> {
self.read_unicode_with_byte_length(string_length * 2).await
}
async fn read_unicode_with_byte_length(&mut self, byte_length: usize) -> TdsResult<String> {
let raw = self.take(byte_length)?;
let units: Vec<u16> = raw
.chunks_exact(2)
.map(|c| u16::from_le_bytes([c[0], c[1]]))
.collect();
Ok(String::from_utf16_lossy(&units))
}
async fn skip_bytes(&mut self, skip_count: usize) -> TdsResult<()> {
self.take(skip_count)?;
Ok(())
}
async fn cancel_read_stream(&mut self) -> TdsResult<()> {
Ok(())
}
fn reset_reader(&mut self) {
self.pos = 0;
}
}
// ---------------------------------------------------------------------------
// Synthetic result-set construction
// ---------------------------------------------------------------------------
fn collation() -> SqlCollation {
SqlCollation {
info: 0x0000_0409,
lcid_language_id: 0x0409,
col_flags: 0,
sort_id: 52,
}
}
/// One column's shape. Metadata and wire bytes are both derived from this, so
/// they cannot drift apart.
#[derive(Clone, Copy, PartialEq, Eq)]
enum ColSpec {
/// `INT`, sent as `IntN` with a 4-byte payload.
Int,
/// `VARCHAR(n)`, sent as non-PLP `BigVarChar`.
Str(usize),
}
impl ColSpec {
fn metadata(self, name: String) -> ColumnMetadata {
match self {
ColSpec::Int => int_column(&name),
ColSpec::Str(len) => varchar_column(&name, len),
}
}
}
fn int_column(name: &str) -> ColumnMetadata {
ColumnMetadata {
user_type: 0,
flags: 0x01, // nullable
data_type: TdsDataType::IntN,
type_info: TypeInfo::var_len(TdsDataType::IntN, 4).unwrap(),
column_name: name.to_string(),
multi_part_name: None,
crypto_metadata: None,
}
}
fn varchar_column(name: &str, len: usize) -> ColumnMetadata {
ColumnMetadata {
user_type: 0,
flags: 0x01, // nullable
data_type: TdsDataType::BigVarChar,
type_info: TypeInfo::var_len_string(TdsDataType::BigVarChar, len, Some(collation()))
.unwrap(),
column_name: name.to_string(),
multi_part_name: None,
crypto_metadata: None,
}
}
/// Column layout of the PoC's benchmark table: 39 `INT` + 9 `VARCHAR(6)`,
/// all nullable.
fn poc_columns() -> Vec<ColSpec> {
let mut cols = vec![ColSpec::Int; 39];
cols.extend(std::iter::repeat_n(ColSpec::Str(6), 9));
cols
}
/// Narrower layout with large string payloads, to weight the value-handoff path
/// (E3) rather than per-column dispatch.
fn wide_string_columns() -> Vec<ColSpec> {
vec![ColSpec::Str(512); 8]
}
fn context_for(specs: &[ColSpec]) -> ParserContext {
let columns: Vec<ColumnMetadata> = specs
.iter()
.enumerate()
.map(|(i, spec)| spec.metadata(format!("col_{i}")))
.collect();
ParserContext::ColumnMetadata(
Arc::new(ColMetadataToken {
column_count: columns.len() as u16,
columns,
..Default::default()
}),
None,
)
}
/// Encodes one column value onto the wire exactly as the server would.
fn push_value(buf: &mut Vec<u8>, spec: ColSpec, row: usize, col: usize) {
match spec {
ColSpec::Int => {
buf.push(4);
buf.extend_from_slice(&((row * 48 + col) as i32).to_le_bytes());
}
ColSpec::Str(len) => {
buf.extend_from_slice(&(len as u16).to_le_bytes());
buf.extend((0..len).map(|i| b'a' + ((i + col) % 26) as u8));
}
}
}
/// Builds `ROWS` ROW tokens with every column present.
fn build_row_stream(specs: &[ColSpec]) -> Vec<u8> {
let mut buf = Vec::new();
for row in 0..ROWS {
buf.push(TokenType::Row as u8);
for (col, spec) in specs.iter().enumerate() {
push_value(&mut buf, *spec, row, col);
}
}
buf
}
/// Builds `ROWS` NBCROW tokens where every 4th column is NULL, so the null
/// bitmap is both present and non-trivial.
fn build_nbcrow_stream(specs: &[ColSpec]) -> Vec<u8> {
let bitmap_len = specs.len().div_ceil(8);
let mut buf = Vec::new();
for row in 0..ROWS {
buf.push(TokenType::NbcRow as u8);
let mut bitmap = vec![0u8; bitmap_len];
for col in 0..specs.len() {
if col % 4 == 3 {
bitmap[col / 8] |= 1 << (col % 8);
}
}
buf.extend_from_slice(&bitmap);
for (col, spec) in specs.iter().enumerate() {
if col % 4 != 3 {
push_value(&mut buf, *spec, row, col);
}
}
}
buf
}
// ---------------------------------------------------------------------------
// Contiguous-buffer writer
// ---------------------------------------------------------------------------
/// Mirrors the shape of `mssql-js`'s `BinaryRowWriter`: every value is appended
/// into one reusable byte buffer rather than becoming an owned `ColumnValues`.
///
/// `DefaultRowWriter` cannot show E3's benefit because it allocates a fresh
/// `Vec` per value either way. This writer can: on the accumulator API the
/// decoder writes straight into `buf`, whereas the old API forces the decoder to
/// assemble a temporary `Vec` first and hand it over to be copied again.
#[derive(Default)]
struct ContiguousRowWriter {
buf: Vec<u8>,
row_start: usize,
}
impl ContiguousRowWriter {
fn new() -> Self {
Self {
buf: Vec::with_capacity(64 * 1024),
row_start: 0,
}
}
}
/// Generates the fixed-width writers the benchmark does not exercise. They must
/// exist to satisfy the trait but never run, so a uniform body is enough.
macro_rules! unused_writers {
($($name:ident($ty:ty)),* $(,)?) => {
$(fn $name(&mut self, _col: usize, val: $ty) { black_box(&val); })*
};
}
impl RowWriter for ContiguousRowWriter {
fn write_null(&mut self, _col: usize) {
self.buf.push(0);
}
fn write_i32(&mut self, _col: usize, val: i32) {
self.buf.push(1);
self.buf.extend_from_slice(&val.to_le_bytes());
}
fn write_string(&mut self, _col: usize, val: SqlString) {
let bytes = val.as_raw_wire_bytes().expect("bench columns must be raw-wire encoded");
self.buf.push(2);
self.buf
.extend_from_slice(&(bytes.len() as u32).to_le_bytes());
let at = self.buf.len();
self.buf.resize(at + bytes.len(), 0);
self.buf[at..].copy_from_slice(bytes);
}
fn write_bytes(&mut self, _col: usize, val: Vec<u8>) {
self.buf.push(3);
self.buf.extend_from_slice(&(val.len() as u32).to_le_bytes());
self.buf.extend_from_slice(&val);
}
fn end_row(&mut self) {
// Stands in for handing the encoded row to the host runtime.
self.buf.clear();
self.row_start = 0;
}
unused_writers!(
write_bool(bool),
write_u8(u8),
write_i16(i16),
write_i64(i64),
write_f32(f32),
write_f64(f64),
write_decimal(DecimalParts),
write_numeric(DecimalParts),
write_date(SqlDate),
write_time(SqlTime),
write_datetime(SqlDateTime),
write_smalldatetime(SqlSmallDateTime),
write_datetime2(SqlDateTime2),
write_datetimeoffset(SqlDateTimeOffset),
write_money(SqlMoney),
write_smallmoney(SqlSmallMoney),
write_uuid(Uuid),
write_xml(SqlXml),
write_json(SqlJson),
write_vector(SqlVector),
);
}
// ---------------------------------------------------------------------------
// Measurement
// ---------------------------------------------------------------------------
/// Decodes the whole buffer once, returning the elapsed time. The decoded values
/// are fed through `black_box` so the work cannot be optimized away.
async fn decode_pass(data: Arc<Vec<u8>>, context: &ParserContext, col_count: usize) -> Duration {
let registry = GenericTokenParserRegistry::default();
let mut reader = MemReader::new(data);
let mut writer = DefaultRowWriter::new(col_count);
let start = Instant::now();
for _ in 0..ROWS {
receive_row_into_internal(
&mut reader,
®istry,
context,
ColumnPolicy::DecodeAll,
&mut writer,
)
.await
.expect("row decode failed");
black_box(writer.take_row());
}
start.elapsed()
}
/// Decodes one buffer and asserts the decoded values match what the builder
/// wrote, and that the stream was consumed exactly. Without this, a harness bug
/// (short reads, wrong wire shape) would silently produce fast but meaningless
/// numbers.
async fn verify_pass(data: Arc<Vec<u8>>, context: &ParserContext, specs: &[ColSpec], nbc: bool) {
let registry = GenericTokenParserRegistry::default();
let mut reader = MemReader::new(Arc::clone(&data));
let mut writer = DefaultRowWriter::new(specs.len());
for row in 0..ROWS {
receive_row_into_internal(
&mut reader,
®istry,
context,
ColumnPolicy::DecodeAll,
&mut writer,
)
.await
.expect("row decode failed");
let values = writer.take_row();
assert_eq!(values.len(), specs.len(), "row {row} column count");
for (col, spec) in specs.iter().enumerate() {
let is_null = nbc && col % 4 == 3;
match (&values[col], spec, is_null) {
(ColumnValues::Null, _, true) => {}
(ColumnValues::Int(v), ColSpec::Int, false) => {
assert_eq!(*v, (row * 48 + col) as i32, "row {row} col {col}");
}
(ColumnValues::String(s), ColSpec::Str(len), false) => {
let expected: Vec<u8> =
(0..*len).map(|i| b'a' + ((i + col) % 26) as u8).collect();
assert_eq!(
s.to_utf8_string(),
String::from_utf8(expected).unwrap(),
"row {row} col {col}"
);
}
(actual, _, _) => panic!("row {row} col {col}: unexpected value {actual:?}"),
}
}
}
assert_eq!(
reader.pos,
data.len(),
"decoder did not consume the stream exactly"
);
}
async fn decode_pass_contiguous(
data: Arc<Vec<u8>>,
context: &ParserContext,
_col_count: usize,
) -> Duration {
let registry = GenericTokenParserRegistry::default();
let mut reader = MemReader::new(data);
let mut writer = ContiguousRowWriter::new();
let start = Instant::now();
for _ in 0..ROWS {
receive_row_into_internal(
&mut reader,
®istry,
context,
ColumnPolicy::DecodeAll,
&mut writer,
)
.await
.expect("row decode failed");
black_box(&writer.buf);
writer.end_row();
}
start.elapsed()
}
/// Same work as [`decode_pass_contiguous`], but the writer is explicitly erased to
/// `&mut (dyn RowWriter + Send)` before the call.
///
/// Before the row-writer generic lands these two are identical — the signature
/// erases either way, so any gap is pure measurement noise and acts as a control.
/// After it lands, the gap between this and [`decode_pass_contiguous`] is the
/// writer-devirtualization headroom that `Box<dyn TdsTransport>` currently blocks
/// from reaching production callers.
async fn decode_pass_contiguous_dyn(
data: Arc<Vec<u8>>,
context: &ParserContext,
_col_count: usize,
) -> Duration {
let registry = GenericTokenParserRegistry::default();
let mut reader = MemReader::new(data);
let mut writer = ContiguousRowWriter::new();
let start = Instant::now();
for _ in 0..ROWS {
let erased: &mut (dyn RowWriter + Send) = &mut writer;
receive_row_into_internal(&mut reader, ®istry, context, ColumnPolicy::DecodeAll, erased)
.await
.expect("row decode failed");
black_box(&writer.buf);
writer.end_row();
}
start.elapsed()
}
fn run_case(name: &str, specs: Vec<ColSpec>, nbc: bool) {
let col_count = specs.len();
let data = Arc::new(if nbc {
build_nbcrow_stream(&specs)
} else {
build_row_stream(&specs)
});
let context = context_for(&specs);
let bytes = data.len();
let rt = tokio::runtime::Builder::new_current_thread()
.build()
.expect("runtime");
rt.block_on(verify_pass(Arc::clone(&data), &context, &specs, nbc));
for _ in 0..WARMUP_PASSES {
rt.block_on(decode_pass(Arc::clone(&data), &context, col_count));
}
let mut samples: Vec<Duration> = (0..MEASURED_PASSES)
.map(|_| rt.block_on(decode_pass(Arc::clone(&data), &context, col_count)))
.collect();
samples.sort();
let median = samples[MEASURED_PASSES / 2];
let best = samples[0];
let ns_per_row = median.as_nanos() as f64 / ROWS as f64;
let rows_per_sec = ROWS as f64 / median.as_secs_f64();
let mb_per_sec = bytes as f64 / median.as_secs_f64() / (1024.0 * 1024.0);
println!(
"BENCH\t{name}\tcols={col_count}\trows={ROWS}\tmedian_ms={:.3}\tbest_ms={:.3}\t\
ns_per_row={ns_per_row:.1}\trows_per_sec={rows_per_sec:.0}\tMiB_per_sec={mb_per_sec:.1}",
median.as_secs_f64() * 1000.0,
best.as_secs_f64() * 1000.0,
);
}
/// `erased` selects the explicitly-`dyn` pass, so the same row shape can be
/// measured through both dispatch styles on one branch.
fn run_contiguous_case_with(name: &str, specs: Vec<ColSpec>, nbc: bool, erased: bool) {
let col_count = specs.len();
let data = Arc::new(if nbc {
build_nbcrow_stream(&specs)
} else {
build_row_stream(&specs)
});
let context = context_for(&specs);
let bytes = data.len();
let rt = tokio::runtime::Builder::new_current_thread()
.build()
.expect("runtime");
let pass = async |d: Arc<Vec<u8>>, c: &ParserContext| {
if erased {
decode_pass_contiguous_dyn(d, c, col_count).await
} else {
decode_pass_contiguous(d, c, col_count).await
}
};
for _ in 0..WARMUP_PASSES {
rt.block_on(pass(Arc::clone(&data), &context));
}
let mut samples: Vec<Duration> = (0..MEASURED_PASSES)
.map(|_| rt.block_on(pass(Arc::clone(&data), &context)))
.collect();
samples.sort();
let median = samples[MEASURED_PASSES / 2];
let best = samples[0];
let ns_per_row = median.as_nanos() as f64 / ROWS as f64;
let rows_per_sec = ROWS as f64 / median.as_secs_f64();
let mb_per_sec = bytes as f64 / median.as_secs_f64() / (1024.0 * 1024.0);
println!(
"BENCH\t{name}\tcols={col_count}\trows={ROWS}\tmedian_ms={:.3}\tbest_ms={:.3}\t\
ns_per_row={ns_per_row:.1}\trows_per_sec={rows_per_sec:.0}\tMiB_per_sec={mb_per_sec:.1}",
median.as_secs_f64() * 1000.0,
best.as_secs_f64() * 1000.0,
);
}
#[test]
fn bench_row_decode() {
println!("BENCH_BEGIN");
run_case("poc_row_39int_9varchar", poc_columns(), false);
run_case("poc_nbcrow_39int_9varchar", poc_columns(), true);
run_case("wide_strings_8x512", wide_string_columns(), false);
run_contiguous_case_with("contig_poc_row_mono", poc_columns(), false, false);
run_contiguous_case_with("contig_poc_row_dyn", poc_columns(), false, true);
run_contiguous_case_with("contig_wide_strings_mono", wide_string_columns(), false, false);
run_contiguous_case_with("contig_wide_strings_dyn", wide_string_columns(), false, true);
println!("BENCH_END");
}
There was a problem hiding this comment.
Pull request overview
Removes boxed-future and dynamic-dispatch overhead from the TDS row-decoding path while preserving protocol behavior.
Changes:
- Converts
SqlTypeDecodeandTdsPacketReaderto RPITIT. - Makes internal row decoding generic over
RowWriter. - Replaces fuzz-only boxed packet readers with a concrete enum.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
mssql-tds/src/token/parsers/row_parser.rs |
Updates decoder test implementations. |
mssql-tds/src/token/parsers/nbcrow_parser.rs |
Updates NBCROW decoder tests. |
mssql-tds/src/token/parsers/common.rs |
Updates the mock packet reader. |
mssql-tds/src/message/prelogin.rs |
Updates the prelogin reader mock. |
mssql-tds/src/io/token_stream.rs |
Genericizes the row-writer decode chain. |
mssql-tds/src/io/packet_reader.rs |
Converts packet-reader methods to RPITIT. |
mssql-tds/src/fuzz_support.rs |
Introduces concrete fuzz-reader dispatch. |
mssql-tds/src/datatypes/decoder.rs |
Converts decoding to RPITIT and boxes SQL_VARIANT recursion. |
mssql-tds/src/connection/transport/network_transport.rs |
Updates the network reader implementation. |
mssql-tds/src/connection/tds_client.rs |
Updates the test transport implementation. |
mssql-tds/fuzz/fuzz_targets/fuzz_tds_client.rs |
Uses the concrete fuzz-reader enum. |
mssql-tds/fuzz/fuzz_targets/fuzz_connection_provider.rs |
Updates provider fuzz construction. |
mssql-tds/fuzz/fuzz_targets/fuzz_connection_provider_network.rs |
Updates network fuzz construction. |
mssql-tds/fuzz/fuzz_targets/fuzz_connection_provider_context.rs |
Wraps the empty reader in the enum. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/coverage Re-triggering the diff-coverage report. The automatic run ( Nothing is blocked by this — |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-tds/src/datatypes/decoder.rs🔗 Quick Links |
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
Review
No blocking findings. Three mechanical commits (async_trait -> RPITIT on SqlTypeDecode and TdsPacketReader, then a generic W: RowWriter through token_stream.rs). I verified that the mechanical parts really are mechanical, and everything the description claims that I could check locally held up. Suggestions and nits inline plus below.
What I verified locally (worktree at cfa0dfe7)
| Check | Result |
|---|---|
cargo fmt --check |
clean |
cargo clippy --workspace --all-features --all-targets -- -D warnings |
clean |
cargo nextest run -p mssql-tds --lib --no-fail-fast |
1619 run, 1615 passed, 4 failed — all 4 are the pre-existing certificate_validator missing-fixture failures (Linux, so the 3 Windows-only win_tls ones do not appear, hence 4 not 7) |
RUSTFLAGS=--cfg fuzzing cargo check -p mssql-tds --lib |
89 warnings — matches your number |
RUSTFLAGS=--cfg fuzzing cargo check --all-targets in mssql-tds/fuzz |
clean |
mssql-py-core (outside the workspace) clippy -D warnings |
clean |
Merge with current origin/main (4 commits ahead, including the ~1600-line tds_client.rs AQE change #206) |
no conflicts, clippy -D warnings clean |
Verified by script rather than by eyeball:
- All 23
FuzzPacketReadermethods delegate to the same-named method on the inner reader and forward every argument — no copy-paste slip anywhere in that 160-line block. - The two
#[cfg]-gated copies ofTdsPacketReaderhave identical method sets in identical order. - No
dyn TdsPacketReaderordyn SqlTypeDecoderemains anywhere in the tree. ioispub(crate), so the "no public API change" claim holds even undercfg(fuzzing).decode_boxedrecursion is bounded at depth 2:decode_zero_propbyte_variantonly re-enters forFixedLengthTypes, and SQL_VARIANT is not one, so a hostile server cannot drive unbounded recursion through the new boxing step.StringDecoder'sreturn-> tail expression is semantically identical.
I did not reproduce the benchmark numbers, since the harness is not in the PR. The magnitude is plausible — a 48-column row was doing 50+ boxed-future allocations per row and very little else — but I am not independently endorsing -61%.
Suggestion: commit 3 (#257), the trade-off is worth naming explicitly
You already flag that the production delta is ~0% and offer to drop resume_row_into_internal. I confirmed the underlying claim: commit 3 required zero edits outside token_stream.rs, and dyn RowWriter + Send still satisfies W via the ?Sized bound. Given four other open PRs touch this file and the 20% is unreachable until the Box<dyn TdsTransport> decision is made, folding commit 3 into #265 — where it can land with a measurable win — is a defensible alternative to shipping it now. Either way is fine and I would not block on it.
One correction in the reassuring direction: your merge-risk table is stale. The branch merges cleanly with today's main and builds clean afterwards.
Nit: trait duplication now costs more per signature
The cfg(not(fuzzing)) / cfg(fuzzing) copies in io/packet_reader.rs are now 23 RPITIT signatures each that have to stay in lockstep. They do today — I checked — but each line got considerably wordier. A macro_rules! taking a $vis parameter would collapse them to one source of truth. Pre-existing duplication and arguably out of scope, so purely optional.
Guard the decode-chain futures. row_fetch_futures_stay_small (#225) builds its futures on TdsClient, which re-boxes at Box<dyn TdsTransport>, so it cannot observe this file. Measured: its four futures are byte-identical on main and on this branch (1128/376/1408/1160) while receive_row_into_internal went 752 -> 1392 B and drive_row_columns 592 -> 1232 B. Add row_decode_futures_stay_small below the boundary, covering both the dyn and monomorphic instantiations against the same 4096 B budget. Move the read_sql_variant comment back onto read_sql_variant; decode_boxed was inserted between them. Add FuzzPacketReader::empty() so both variants have a constructor, which drops the last EmptyReader import from the fuzz targets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41e46220-ad76-4a77-b477-e768951dcf92
|
Thanks — the future-size catch was the valuable one and it is fixed in Commit 3 (#257): keeping it, but your framing is now in the bodyI took the suggestion seriously and came down on keeping it, for reasons that are about cost rather than value:
What I did take from this is that the body undersold the trade-off, so it now says plainly that commit 3 is ~0% in production today and why. If you would still rather it moved, say so and I will split it — the argument above is about ordering cost, not about it being wrong.
|
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
Re-review of 4c64fd2f — approving
All three inline findings are resolved correctly, and the responses on the two decisions are sound. No remaining findings.
Verified on 4c64fd2f
| Check | Result |
|---|---|
cargo fmt --check |
clean |
cargo clippy --workspace --all-features --all-targets -- -D warnings |
clean |
cargo nextest run -p mssql-tds --lib --no-fail-fast |
1620 run, 1616 passed, 4 failed — the same pre-existing certificate_validator fixture failures (Linux, so the 3 Windows-only win_tls ones do not appear); the +1 versus my previous run is the new guard |
RUSTFLAGS=--cfg fuzzing cargo check --all-targets in mssql-tds/fuzz |
clean, 89 lib warnings unchanged |
Merge with origin/main @ 445459bb |
no conflicts, clippy -D warnings clean on the merged tree |
The new guard is not vacuous. I instrumented row_decode_futures_stay_small to print rather than just assert:
receive_row_into_internal (dyn) = 1416 B
receive_row_into_internal (mono) = 1352 B
drive_row_columns (dyn) = 1256 B
drive_row_columns (mono) = 1208 B
resume_row_into_internal (dyn) = 1408 B budget 4096 B
Roughly 2.9x headroom, and covering both instantiations plus resume_row_into_internal is more than I asked for. Asserting <= MAX rather than exact equality is the right call: your figures (752/1392, 592/1232) differ slightly from mine (760/1416, 600/1256) because these are toolchain and platform dependent, so an equality assert would have been a CI flake generator.
Both nits are clean — the SQL_VARIANT comment is back above read_sql_variant, and FuzzPacketReader::empty() drops the EmptyReader import at the call site as predicted.
On the two decisions
Commit 3 (#257) — accepting your reasoning, suggestion withdrawn. The ordering-cost argument is the right frame and I had not weighted it properly: deferring means rewriting the same four signatures a second time against a token_stream.rs that #238/#245/#186/#215 will have moved.
Macro for the #[cfg]-gated trait copies — agreed, do not do it. The rust-analyzer / doc-generation / error-span argument is the convincing one.
One factual correction for the record, since it is load-bearing in your first bullet: the two copies do not carry different bounds. I diffed the two trait bodies — the entire difference is two #[allow(dead_code)] attributes (on read_u16_varbyte and read_unicode) plus pub(crate) versus pub on the trait itself. A $vis macro would in fact have collapsed them. The conclusion still holds on the tooling argument alone; only that one premise is off.
One small note
Your comment says the body "now says plainly that commit 3 is ~0% in production today and why", but the description diff shows no change in that section. Nothing was lost — the original body already said it explicitly ("the win is not realized through the public API today" / "#257's production delta is ~0% for structural reasons"). Flagging only in case a further edit was intended and did not land.
Approval scope
Approving on the code. CI on this head was still partly pending when I looked (8 pass, 10 pending) — please confirm the full ADO validation pipeline goes green before merging, since the JS yarn testci leg and the live-server integration tests are the parts I cannot run locally.
Resolves a conflict in the decoder import block: #237 added the BigUint and ToPrimitive imports next to `use async_trait::async_trait;`, which this branch deleted when it removed the attribute from SqlTypeDecode. Kept both new imports and left async_trait out, since the file no longer references it. The decimal reassembly rewrite from #237 merged cleanly into the de-async_trait signatures and needs no further adaptation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41e46220-ad76-4a77-b477-e768951dcf92
Description
Removes per-row and per-column dispatch overhead from the TDS row-decode path. Three
commits, all rewriting the same signatures in the same call chain:
Why one PR instead of three. Splitting them means rewriting those exact signatures
three times with two rebases in between. #251 measures ~0% on its own, so it has no
standalone justification — it exists to unblock #257. They are kept as three separate
commits so the PR stays bisectable and reviewable commit by commit.
1eaa9536— Removeasync_traitfromSqlTypeDecode(#251)#[async_trait]boxes every returned future, so each column decode allocated. The traitnow returns
impl Future<Output = ...> + Senddirectly.decode_boxedis added for the SQL_VARIANT recursion — a nativeasync fnthatrecurses into itself is
E0733without an explicit boxing step.+ Sendis spelled out on the return type; bare AFIT does not promise it, and thecallers need it.
StringDecoder'sreturn Ok(ColumnValues::Null)becomes atail expression. This is required, not a drive-by — under
#[async_trait]the bodylived inside
Box::pin(async move { ... }), so thereturnwas not a tail expression.As a native
async fnit is, andclippy::needless_returnfires. Without this hunk thecommit fails
cargo bclippyin isolation.e977feb9— ConvertTdsPacketReaderto RPITIT (#252)The widest change and the one that carries the PR: ~22 methods across both cfg-gated
trait definitions move from
#[async_trait]to-> impl Future<...> + Send.impl TdsPacketReader for Box<dyn TdsPacketReader + Send + Sync>blanket impl isdeleted — RPITIT is not dyn-compatible, and nothing needed the boxed form once the
concrete readers were threaded through.
PlpChunkStreamReader/PlpColumnStream: seven&mut dynparameters become ageneric
T.fuzz_support.rsstored threeBox<dyn TdsPacketReader>. Those become a two-variantFuzzPacketReaderenum;FuzzReaderandEmptyReaderwere the only types ever boxedthere, and only under
#[cfg(fuzzing)]. The fourfuzz_targets/*.rsconstructionsites are updated to match.
cfa0dfe7— Make the row-decode chain generic over theRowWriter(#257)GenericDecoder::decode_into<T, W> where W: RowWriter + ?Sizedwas already generic. Thewriter calls were still vtable calls purely because every caller upstream coerced to
&mut (dyn RowWriter + Send). Four functions inio/token_stream.rsnow takeW: RowWriter + Send + ?Sizedandwriter: &mut W:drive_row_columns,decode_or_decrypt_column,receive_row_into_internal, andresume_row_into_internal.resume_row_into_internal(the pull-cursor twin) is included for symmetry. It is free —its only callers pass
dyn, so it adds no extra instantiation. Happy to drop it if areviewer prefers the tighter diff.
Object safety. Because the bound is
?Sized,dyn RowWriter + Sendstill satisfiesW. Every existing caller compiles unchanged — this commit required zero editsoutside the one file — and the public
TdsResultSet::next_row_into(&mut (dyn RowWriter + Send))stays dyn-compatible.Where the writer's type is actually erased
Tracing the chain end to end:
Both FFI consumers already hold a concrete writer and coerce to
dynonly because thesignature demands it. But the erasure that matters is one level lower:
TdsTokenStreamReaderis used as a trait object — transitively, as a supertrait of theboxed
TdsTransport. A grep fordyn TdsTokenStreamReaderfinds nothing because theboxing is spelled
Box<dyn TdsTransport>.Two obvious ways to recover the concrete writer both fail:
next_row_into_typed<W>(..) where Self: Sizedsibling — aSelf: Sizedmethodcannot be called on a trait object, and
get_next_row_intomust call throughBox<dyn TdsTransport>. The typed method would be uncallable at the one place thatneeds it.
Self: Sizedmakes
TdsTransportdyn-incompatible, soBox<dyn TdsTransport>stops compiling. It isthe core of the connection architecture and is also produced in four places in
tds_connection_provider.rs.So below the transport boundary
Wcan only ever instantiate asdyn RowWriter + Send,and #257's production delta is ~0% for structural reasons, not incidental ones.
Shipping it anyway is deliberate: it costs 12 lines and zero risk, it closes #257 with a
measured answer rather than a guess, and it leaves the entire chain below the transport
boundary ready so the win lands for free when that boundary is removed. The blocked
headroom is quantified below rather than left speculative, and filed as a follow-up (#265).
Measured results
Method: in-crate
#[cfg(test)]microbenchmark against an in-memory reader, 20,000 rowsper pass, 3 warmup + 9 measured passes, median per run; 3 runs per branch, median of
medians reported. Four throwaway local branches stacked one commit at a time from
main. Quiet machine, no concurrent builds. Run-to-run noise is ±1–2%. Numbers werere-measured on this branch, not copied from #247.
Criterion is not usable here:
receive_row_into_internalispub(crate)and a bench is aseparate crate, and
mssql-mock-tdscaps a result set at ~200 rows via au16lengthcast. The harness runs a verification pass first, asserting every value round-trips and
the reader lands exactly at end-of-buffer, so a harness bug cannot produce fast-but-wrong
numbers. Full harness and repro steps are in a follow-up comment.
What these numbers do and do not cover. The harness enters at
receive_row_into_internal, so every figure here measures the decode path below thatpoint. Anything above it is excluded from both the baseline and the result — notably the
CancelHandle::run_until_cancelledwrapper (core.rs:43), and the per-rowtokio::time::timeouton top of it when a request timeout is set. That makes these numbersa clean isolation of the dispatch change, but not an end-to-end row-throughput prediction.
Those excluded layers are measured separately in #271, which finds the timeout — not the
cancellation wrapper — is where the time goes, and that today only
mssql-py-corepays it(
cursor.rs:72hardcodestimeout: Some(30); the other consumers leave itNone, whichtimeout_to_durationattds_client.rs:519turns into no timeout arm at all). They areunmeasured here rather than measured and dismissed.
Median ms per 20,000-row pass (lower is better):
#251 alone: ~0% (+5.5% / −5.6% / +2.8% / +1.1% across cases — all inside noise; one
run produced a 213.9 ms outlier against a ~156 ms baseline, which is exactly why the
protocol is median-of-3-runs). Confirms the previously reported standalone result. It
ships for API hygiene and as the #257 enabler.
#252 vs
main: this is nearly the entire win.−61.0% on the 39 INT + 9 VARCHAR(6) row reproduces the −61.6% reported on #247.
Cumulative,
main→ all three commits, through the realdynproduction path:Quantifying the headroom #257 cannot reach
The contiguous-writer case (the shape
mssql-js'sBinaryRowWriteruses) is measuredtwice on each branch — once passing the concrete writer, once with an explicit
as &mut (dyn RowWriter + Send)coercion. Before #257 the two are identical byconstruction, so they act as a noise control:
Controls stay within ±2%; #257 opens a 20.0% gap. That 20.0% is real, reachable, and
currently blocked by
Box<dyn TdsTransport>— no production caller can get it today.This quantifies the dispatch headroom specifically — see "What these numbers do and do
not cover" above for what sits outside the harness.
Filed as #265 with this number attached so the work is justified rather than
speculative.
Validation
cargo bfmtcargo bclippy-D warnings).\scripts\bfmt.ps1/.\scripts\bclippy.ps1mssql-py-core, which is excluded from the workspace and implementsRowWritercargo btestmain(see below)cargo nextest run -p mssql-tds --libmain, +1 test is the new guard (see below)RUSTFLAGS=--cfg fuzzing cargo check -p mssql-tds --libmainand this branch — no regressionRUSTFLAGS=--cfg fuzzing cargo check --all-targetsinmssql-tds/fuzzcargo build -p mssql-js --releasemssql_js.dll)cargo build -p mssql-odbc --releaseThe fuzzing legs matter disproportionately here:
fuzz_support.rsis#[cfg(fuzzing)], soa default
cargo check— andcargo bclippy, which does not set the cfg — compiles noneof it. #252 rewrites the reader storage in that file, and the cherry-pick initially
introduced three
missing_docswarnings that only the explicit fuzzing leg caught. Thoseare fixed. (
MockTransport::newis still undocumented; that is pre-existing onmain.)row_fetch_futures_stay_smallasserts on a future built with a concreteDiscardRowWriter, so commit 3 might have been expected to change what it measures.All four futures are byte-identical to
main:next_row_cursorread_row_columndrain_rowsget_next_row_intoThat identity is not evidence the decode chain is unchanged — it is evidence the guard
cannot see it. Those four futures are built on
TdsClient, which re-boxes atBox<dyn TdsTransport>; nothing below that boundary can propagate into them. Below itthe futures nearly doubled:
receive_row_into_internal(dyn)drive_row_columns(dyn)Well inside budget, and expected — RPITIT inlines what
Box::pinused to keep on theheap, which is the entire point of #252. But it was unguarded, so commit 4 adds
row_decode_futures_stay_smallintoken_stream.rs, covering both thedynandmonomorphic instantiations against the same 4096 B budget. Caught by David Engel (@David-Engel) in
review; #225 added the original guard for exactly this failure mode.
CI results
The full Azure DevOps validation pipeline went green on
4c64fd2f(the pre-merge head), 19/19 — all 5 build platforms(Linux, Linux ARM, Windows, Windows ARM, macOS), Test macOS, the SQL host stage, Kerberos
authentication tests, and all three cross-repo
mssql-pythonlegs.Three of those close gaps that could not be closed locally:
direct evidence that the 359 locally-failing integration tests (see below) fail purely
because this machine has no server to connect to, not because of anything in this change.
mssql-pythonbuild and themssql-pythonsuite on themssql-odbcdriver both pass, exercising the PyO3 binding end-to-end rather than just compiling it.
yarn install,yarn build,yarn buildapi, thenyarn testciagainst a live SQL Server), which isthe one thing I could not run locally. See the FFI section below.
It is re-running now on the merge commit
6cf02aa8; results will be updated here once itsettles.
Diff coverage: 98% — 123 changed lines, 2 missing; overall 91.5%. Both missing lines
are non-executable, and are artifacts of how diff-cover attributes lines rather than
untested logic:
decoder.rs:364— thewhere T: TdsPacketReader + Send + Sync,continuation line ofPlpChunkStreamReader::skip_to_end's signature, which this PR rewrites from&mut dynto a generic. The body is covered:
plp_chunk_stream_reader_skip_to_end_flushes_remaining_chunksexercises it directly, there are two further test call sites, and production calls it at
token_stream.rs:434.decoder.rs:3254— a blank line immediately insidemod decode_into_tests {.The counted diff grew from 58 lines to 123 when
mainwas merged in: #237 rewroteread_decimalin the same file, so this branch's de-async_traithunks now sit againstlarger surrounding functions. No logic was added. Everything executable in the change is
covered, which is the expected shape for a pure dispatch refactor — every touched line is
already on a path the existing tests exercise, and that is also why no new tests are added
here.
FFI bindings
#252 changes a trait these depend on transitively, so all three were checked:
mssql-py-core(PyO3, implementsRowWriter) —cargo check+ clippy clean viascripts\bclippy.ps1. It is outside the workspace, so the plain aliases miss it.mssql-js(NAPI, implementsRowWriter) — release build produces the cdylib locally.yarn install/yarn build/yarn testcould not be run on my machine:yarn installfails with a TLS handshake error reaching the npm registry from this environment.
CI covers this gap completely. The Build Linux stage runs
enableJsBuild: trueandenableJsTest: true, executingyarn install,yarn build,yarn buildapiand thenyarn testciagainst a live SQL Server — the JS test step is gated onBuild.Reason == PullRequest, so it runs precisely on PR builds like this one. That stageis green, so the TypeScript/AVA half is verified after all.
mssql-odbc— release build passes. It consumes rows through a different path anddoes not implement
RowWriter; confirmed untouched.Pre-existing test failures — not introduced here, and not fixed here
Two different scopes, so both are reported rather than just the flattering one.
Workspace. Re-measured on both refs after merging
maininto this branch:main068efe7a6cf02aa8Not just the same failure count — the same tests. Both failing sets were extracted and
diffed name-for-name: symmetric difference 0. The one extra test and extra pass on this
branch is
row_decode_futures_stay_small, the guard added in commit 4. (The earlier figurein this section, 364, was measured against the older base
e40e779d;mainhas since addedtests of its own, which is where the other two came from — not from this change.)
Of those 366, 7 are
mssql-tdslib unit tests (listed below) and the remaining 359are integration tests under
tests/that need a live SQL Server; both sides split7 + 359 = 366. In this environment
they fail during connect with
Schannel TLS handshake failed: SEC_E_WRONG_PRINCIPAL (0x80090322)— e.g.test_always_encrypted(41),test_cursor_ops(39),test_rpc_datatypes(33),test_bulk_copy(21). That is an environment limitation, not a signal about this change;CI runs those against a real server.
Library only (
cargo nextest run -p mssql-tds --lib).main1745 run / 1738 passed /7 failed; this branch 1746 / 1739 / 7 — the clean comparison, since it excludes everything
needing a server. Same 7 tests, with the +1/+1 again being the new guard:
connection::transport::certificate_validator::tests::test_load_certificate_from_pem,test_load_certificate_from_der,test_is_certificate_expired_valid,test_pem_and_der_certificates_produce_same_der— allCertificateNotFound { path: "tests/test_certificates/..." }, i.e. missing fixtures.connection::transport::win_tls::validate::tests::validate_pinned_cert_matches_identical_der,validate_pinned_cert_mismatch_is_pin_error,validate_pinned_cert_missing_pin_file_is_pin_error.Deliberately left alone — unrelated to this change.
On shipping the benchmark harness
The harness is not in this PR, and that was a judgement call rather than a default.
In favour of shipping: reviewers and future perf work get a stable baseline instead of
re-deriving one; it is
#[cfg(test)], so zero production cost; #247 asks each PR to carryits own benchmark; and it covers a contiguous-buffer writer shape no existing test does.
Against, and decisive: it is ~660 lines of self-described throwaway test code, roughly
twice the size of the actual change. Under
cargo btest— llvm-cov-instrumented and not--release— 1.2M row decodes would blow nextest'sslow-timeout(60 s, terminate after3 → 180 s hard kill) and fail CI. Marking it
#[ignore]fixes CI but leaves 660 lines ofnever-executed code, which the repo's "no AI slop" convention disfavours. The numbers stay
fully reproducible either way because the harness and exact repro steps are attached to the
benchmark comment below. Happy to land it as a separate PR under #258 if reviewers want a
durable baseline.
Merge risk
Commits 2 and 3 modify
mssql-tds/src/io/token_stream.rs. Four other open PRs also touchthat file, none of them on
maintoday:token_stream.rs#238 is the PoC this work derives from and has the largest diff on the file. #245 reworks
NBCROW null-bitmap handling, which this PR's NBCROW benchmark cases exercise directly.
Whichever of these lands first forces the others to rebase this file. Noted here so
reviewers weighing merge order do not have to check for themselves.
mainhas since conflicted, and the conflict is resolved. The earlier claim that therewas no risk against
mainwas accurate when measured at445459bb, and stopped beingaccurate afterwards.
mainadvanced eight commits to068efe7a, and #237 — "Guard decimalmagnitude reassembly against 128-bit shift overflow" — rewrote
read_decimalindatatypes/decoder.rs, the same file commit 1 de-async_traits. GitHub flipped the PR toCONFLICTING/DIRTY.Resolved by merging
maininto the branch in6cf02aa8. Exactly one hunk conflicted,and it was the import block rather than any logic: #237 added
BigUintandToPrimitivedirectly adjacent to the
use async_trait::async_trait;line that commit 1 deletes, so thetwo sides edited touching lines. Kept both new imports, dropped
async_trait— verified tooccur 0 times in the merged file, since commit 1 removes every attribute that used it.
#237's decimal rewrite itself needed no adaptation: its new
reader.read_bytes(&mut magnitude[..magnitude_len]).await?already satisfies the RPITITsignature introduced by commit 2.
Merged rather than rebased, deliberately. Recent
mainhistory is linear single-parent, sothe repo squash-merges feature PRs and this merge commit is erased at merge time rather than
landing on
main; and a force-push would stale the three resolved inline review threadswhile the review is still open. Post-merge re-validation (fmt, clippy including
mssql-py-core, both fuzzing legs, full workspace suite, and both future-size guards) isrecorded in the Validation section above — every future-size number in this PR was
re-measured against the new
mainand is byte-for-byte unchanged.Related Issues
Fixes #251
Fixes #252
Fixes #257
Part of #247.
Follow-up: #265 — the blocked dispatch headroom quantified above. Not addressed here; it needs the
Box<dyn TdsTransport>boundary decision.Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses — 2771 run, 2405 passed, 366 failed, 11 skipped, with thefailing set name-for-name identical to
main(symmetric difference 0). 7 arepre-existing lib failures; the other 359 are integration tests needing a live SQL
Server, and they pass in CI, which has one. See the note above.
passing tests are the regression net, and diff coverage on the change is 98%
(123 lines, 2 missing — both non-executable: a
where-clause continuation line anda blank line; see the CI results section); benchmark evidence is attached below
TdsPacketReaderandSqlTypeDecodearepub(crate);next_row_into's signature is untouched andstill dyn-compatible