Arm the per-row request timeout lazily: ~3% on `main`, ~15% after #252

[saurabh500]

Read this before comparing to #265. The two headline numbers belong to different
consumers and never sum on one path. #265's measured 20.0% is on the contiguous-writer
shape mssql-js uses; this issue's ~42% is paid only by mssql-py-core, which is the one
caller that sets a request timeout. Triaging by number size alone will stack them and be
wrong.

Problem statement

NetworkTransport::receive_row_into wraps every single row decode in two combinators
(network_transport.rs:1388-1398):

let cancellable = CancelHandle::run_until_cancelled(
    cancel_handle,
    receive_row_into_internal(self, &*PARSER_REGISTRY, context, plan, writer),
);
let result = match remaining_request_timeout.as_ref() {
    Some(t) => match timeout(*t, cancellable).await { ... },
    None => cancellable.await,
};

Measured on the in-crate row-decode harness (decode_bench.rs, 20,000-row passes,
3 warmup + 9 measured → median, --release), 48-column row (39 INT + 9 VARCHAR(6)),
ContiguousRowWriter. All four modes share one runtime and one warmup regime, so the
comparison is paired:

Mode	median ms	ns/row	vs bare
bare (receive_row_into_internal direct)	32.811	1640.5	—
run_until_cancelled(None, ..)	34.709	1735.4	+5.8%
run_until_cancelled(Some(&h), ..)	34.415	1720.7	+4.9%
+ tokio::time::timeout(..)	46.751	2337.6	+42.5%

Arming a tokio::time::timeout per row costs ~36% on top of the cancellation wrapper,
and ~42% over an unwrapped decode. That is a timer registration and deregistration for
every row in a result set. The cancellation wrapper itself is comparatively cheap (~5%,
close enough to the noise floor to be worth treating as "small" rather than as a precise
figure).

Note run_until_cancelled(None, ..) costs essentially the same as Some(..) — the cost is
the wrapper existing, not a handle being supplied. Connections with no cancel handle still
pay it.

The byte-size picture, and why it is not the headline

This was found via cargo +nightly rustc -- -Zprint-type-sizes, which paints a different
picture from the timings — worth recording precisely so nobody optimizes the wrong thing:

Layer	bytes	Δ
receive_row_into_internal (the actual decode)	1328	—
+ tokio_util CancellationToken::run_until_cancelled	2744	+1416
+ core::CancelHandle::run_until_cancelled	4088	+1344
+ tokio::time::Timeout	4200	+112
+ row-fetch wrapper	4416	+216

By size, cancellation is ~70% of the future and the timeout is nearly free (+112 B). By
time, that is exactly inverted: cancellation is ~5% and the timeout is ~36%. Size and
cost are anti-correlated here, so the byte table should not be used to prioritize.

The size growth does have a real cause worth fixing on its own terms. In core.rs:50-56,
f is awaited in both match arms:

Some(handle) => handle.cancel_token.run_until_cancelled(f).await,  // stores f
None => f.await,                                                    // stores f again

These are separate suspend points in separate arms, and rustc stores both rather than
overlapping them: 2744 + 1328 = 4072 against a measured 4088. By contrast the
timeout/cancellable pair one level up does overlap correctly (4088 → 4200), because
timeout() is a plain struct constructor rather than an async fn, so the move is visible
to layout.

Why the size matters despite not being the hot cost: at ~1.4 KB the row future fits under
the repo's own 4096 B row_fetch_futures_stay_small guard (tds_client.rs:4418)
unboxed, which would let #[async_trait]'s per-row Box::pin be removed outright. At
4416 B it cannot. See #265 for the measurements showing all four row futures blowing that
budget when the box is removed naively.

Proposed solution

Roughly in order of value:

1. Stop arming a timer per row. The request timeout is a property of the request, not
   of each row. Compute a deadline once and either check it between rows, or hold a single
   sleep_until for the whole result set, or scope the timeout to the network read rather
   than the decode.
2. Make CancelHandle::run_until_cancelled await f exactly once, so the two arms
   share a slot instead of both being stored — e.g. select over a cancellation branch that
   is pending() when no handle is supplied. This is the ~2.7 KB, not the ~36%.
3. Check cancellation between rows rather than inside them. A row decode is
   microseconds; is_cancelled() between rows costs zero future bytes and no timer.

(1) and (3) together would remove both wrappers from the per-row path entirely.

Measured: conditional arming works, and the wrapper form matters

Spiked on dev/saurabh/row-decode-perf-feasibility @ 3371dc58. Two new ladder rungs
arm the timer only if the decode does not complete on its first poll — Timeout::poll
polls the inner future before the delay, so a row served from a buffered packet never
observes the timer either way, and skipping the arm is observationally identical.

All six modes in one binary, one runtime, one warmup regime; 9 outer runs, min reported
(robust to upward interference on a shared machine):

Mode	min ms	vs bare	vs cancel_some
bare	30.48	—	−6.6%
cancel_none	31.26	+2.6%	−4.2%
cancel_some	32.63	+7.1%	—
+ timeout(..) (today)	37.85	+24.2%	+16.0%
+ lazy timeout, async fn helper	34.20	+12.2%	+4.8%
+ lazy timeout, inline	32.72	+7.3%	+0.3%

The timer's own cost is the gap between cancel_some and + timeout — 17.1 points.
Inline lazy arming lands within 0.3% of cancel_some, i.e. it removes ~99% of that
cost. What remains at +7.1% is the cancellation wrapper, which is items (2)/(3) above and a
separate fix.

The helper form leaves ~30% of the win on the table. Writing it as
async fn lazy_timeout(..) costs 4.5% over the inline form, reproducibly and paired.
Same cause this issue already documents for run_until_cancelled: an async fn with an
await in one match arm stores that arm's future in the state machine, so the per-row
future grows and is memcpy'd on every move. Implement this inline at the call site, or as
a hand-written Future — not as an async fn wrapper.

Worth noting how the helper cost was found, since it nearly wasn't: the first measurement
of the inline form and the later measurements of the helper form were taken in different
builds, which made a real 4.5% regression look like machine noise. Only re-measuring all
six modes in one paired ladder separated them.

The exhausted-budget trap is covered

Per the caveat above, conditional arming inverts semantics if it keys on the budget being
zero. It must key on suspension. lazy_timeout_matches_eager_timeout_on_exhausted_budget
asserts, for Duration::ZERO:

inner future	tokio::time::timeout	lazy arming
ready on first poll	Ok	Ok
suspends	Elapsed	Elapsed

The second row is the guard: skipping the timer because "the budget is gone" would turn a
fetch that must fail into an unbounded wait.

Caveat

The harness reads from memory, so the inner future never suspends and the lazy path never
arms. That is the intended isolation, and it is the common case (rows served from an
already-buffered packet), but a workload that genuinely suspends per row arms the timer as
before and pays today's cost plus one extra poll. The win is real and workload-shaped, not
universal.

Affected crate

mssql-tds

Alternatives considered

Doing nothing. This is pure per-row overhead on the hottest path in the driver, and it is
larger than the dispatch win being pursued in #247 / #265 — that work chases a measured
20.0% writer-devirtualization gap, while the timeout alone is ~42% on the same shape of
row.

Raising the 4096 B future-size guard instead of shrinking the future. Rejected: a ~5.5 KB
future is not free just because it is on the stack — it is memcpy'd on every move — so the
guard is doing real work and the right response is to shrink the payload.

Additional context

Found while spiking #265. Independent of it: #265 removes the indirect call on the row
path, this removes wrapper overhead, and the two compose.

Reproduce with the run_wrapper_ladder case added to mssql-tds/src/decode_bench.rs on
dev/saurabh/row-decode-perf-feasibility:

cargo test --release -p mssql-tds --lib decode_bench::bench_row_decode -- --nocapture

Two caveats on the measurement, stated so they are not discovered later as surprises:

• The harness reads from an in-memory buffer, so the decode never suspends on I/O. This
  isolates timer arm/disarm overhead, which is the intent, and it is representative of the
  common case where rows are served from an already-buffered packet. A workload that
  genuinely suspends per row would amortize differently.
• The ~5% cancellation figure is near this harness's noise floor (the same workload
  measured through two different case functions differs by ~4%). Treat it as "small", not
  as a precise number. The ~42% timeout figure is far outside that band.

Which consumers actually pay this

The timeout arm is only entered when remaining_request_timeout is Some, and
TdsClient::timeout_to_duration maps both None and Some(0) to None. So the cost
is opt-in, and today exactly one consumer opts in:

Consumer	what it passes	pays the ~42%?
mssql-py-core	ExecuteOptions { timeout: Some(30), .. } — hardcoded, cursor.rs:72	yes, always
mssql-js	() → ExecuteOptions::default() → timeout: None	no
mssql-odbc	no timeout plumbing at all	no
mssql-tds-cli	no timeout plumbing at all	no

This sharpens the issue rather than shrinking it. It is not "the row path costs 42%" — it is
the Python path costs 42% and the others cost nothing, from a single hardcoded literal
that no Python caller can override. Note the asymmetry with #265, whose measured 20.0% is on
the contiguous writer shape that mssql-js uses: the consumer that stands to gain most from
#265 is not the one paying here, and vice versa.

Two things follow. First, cursor.rs:72 is worth questioning on its own merits — a
non-configurable 30 s query timeout will abort any legitimately long-running Python query,
independent of performance. Second, whoever benchmarks this should set the timeout
explicitly rather than relying on defaults, or they will measure the None path and
conclude there is nothing here.

What the timeout currently means, and what a fix must preserve

Worth stating before anyone hoists the timer, because the current semantics are not the
obvious ones. get_next_row_into (tds_client.rs:3115-3127) runs:

loop {
    let start = Instant::now();
    let result = self.transport.receive_row_into(.., self.remaining_request_timeout, ..).await?;
    self.update_remaining_timeout(start);

So the per-row timeout() is not a fresh 30 s per row. It is a single decrementing
operation budget, re-armed each row with whatever remains, and update_remaining_timeout
charges only the time spent inside the transport await. Time the caller spends between
next_row_into calls is never charged.

That makes the naive fix — hoist to one wall-clock deadline computed at operation start —
not semantics-preserving. It would additionally charge consumer think-time, so a slow
consumer over a large result set could begin tripping a timeout it never used to. For the
one consumer that pays this today (Python, driving row-by-row through PyO3) that is exactly
the wrong population to silently start charging.

Two further details for whoever implements it:

• update_remaining_timeout saturates to Duration::ZERO, not None (:531-538), so an
  exhausted budget keeps the wrapper armed. It does not follow that such a row fails
  immediately: Timeout::poll (tokio 1.53.1, time/timeout.rs:211-221) polls the inner
  future first and returns Ok before the delay is ever polled. A row already served from
  a buffered packet therefore still succeeds on a zero budget; the Elapsed only fires on a
  fetch that actually suspends.

  This is what makes conditional arming provably correct rather than merely safer. The
  only case where the wrapper is observable is exactly the case in which it would still be
  armed. It is also a trap: Some(ZERO) looks like a degenerate state worth optimizing away
  ("budget is gone, skip the wrapper"), and doing so inverts the semantics on the suspending
  path — a fetch that should fail with Elapsed would instead wait unbounded. Any
  conditional-arming change needs a test asserting that an exhausted budget still times out a
  suspending fetch.

• It shares a budget with deduct_timeout (:619), which is easy to mis-scope in either
  direction. It is not a separate budget: at all 18 call sites (10 in cursor_ops.rs,
  8 in tds_client.rs) its result is converted by timeout_to_duration and assigned to the
  very same remaining_request_timeout field this issue is about. One budget, two units
  (Option<u32> seconds vs Option<Duration>), two moments — deduct_timeout computes the
  value after a reconnect, and the row loop decrements it from there.

  The two moments disagree about zero, which is the substantive part:

  budget reaches zero	representation	timer armed?
  at init, via deduct_timeout	Some(0) → timeout_to_duration → None	no — request unbounded
  per row, via update_remaining_timeout (:531)	Some(Duration::ZERO)	yes — fails fast

  So an exhausted budget runs forever if it was exhausted by a reconnect, and fails fast if it
  was exhausted by row fetching. The round-up at :623-631 turns any non-zero sub-second
  reconnect into a whole second deducted, so a 100 ms reconnect against a 1 s timeout already
  reaches the first case. Filed as Request timeout is silently dropped when a reconnect precedes it: deduct_timeout produces Some(0), which both sinks treat as "no timeout" #272 — not blocking the work here, but it is this same
  field arriving at zero by a different route, so a fix to either should leave the two
  agreeing.

The constraint this implies is the useful part. The measured cost is arming and disarming
a timer-wheel entry per row, not the budget bookkeeping — which is a cheap subtraction. So a
fix can keep the decrementing-budget semantics exactly and still remove the cost, by keeping
the Instant arithmetic and only arming a real timer when the fetch would actually suspend.
That is strictly better than hoisting, because it changes nothing observable.

[saurabh500]

Correcting the headline figures in this issue, and adding the finding that matters more than the correction

I filed this with numbers from a 4-rung ladder that had a design flaw. Both figures in the title are wrong. Correcting them here, and retitling.

The flaw

The ladder ran all passes of one mode before moving to the next, so machine drift landed entirely on whichever mode happened to be running. The tell is in the published numbers:

bare          32.811 ms
cancel_none   34.709
cancel_some   34.415   <- 0.294 ms FASTER than cancel_none
timeout       46.751

cancel_some does strictly more work than cancel_none and cannot be faster. That ordering is physically impossible, so at least 0.294 ms of drift is present between adjacent rungs.

What survives, and what I'm no longer quoting

The timeout rung survives. Its effect is 46.751 − 34.415 = 12.336 ms against a drift tell of 0.294 ms — a ratio of 42×. Drift of that size cannot manufacture an effect that large. Bounding by the full observed drift in the worst direction gives 34.7–37.0%, not 42%.

The ~5% cancellation figure I'm withdrawing from the title. cancel_none − bare = 1.898 ms is only 6.5× the drift tell, and the tell is a lower bound on drift rather than a measurement of it. That margin is too thin to quote from a design that produced an impossible ordering. It needs a paired re-measurement before it goes back in; I'm not asserting it's wrong, I'm asserting it isn't established.

The larger correction: the number depends on the branch, and that's the interesting part

The ladder above ran on my E1–E7 spike branch, which has #[async_trait] removed from TdsPacketReader — i.e. it already contains #252. On main the same effect is far smaller:

branch	inner future	no-timer decode	timeout cost	% of row
main	2360 B	~6570 ns/row	~208 ns/row	~2.6–3.1%
spike (has #252)	4088 B	~1720 ns/row	~617 ns/row	~35%

The main figure is from a paired ABBA A/B design (41 pairs × 20 000 rows, 7 independent processes), not the ladder, and it is writer-independent as it must be (207.9 ns with DiscardRowWriter, 202.1 ns with DefaultRowWriter — only the denominator moves).

Why they differ, and why it compounds. tokio::time::timeout takes its future by value, so the per-call cost scales with the inner future's size. On main, async_trait boxes every reader await, so the outer state machine holds pointers — small future, slow decode (a heap allocation per read). Removing it inlines the state machines — bigger future, faster decode. Numerator and denominator move in opposite directions, so this issue gets more valuable after #252, not redundant.

Consequence for sequencing

• Convert TdsPacketReader to RPITIT to remove per-read boxing (row decode perf, axis: dispatch) #252 first. It is worth ~2.6× on its own, and it raises this issue from ~3% to ~35%.
• This issue is worth doing either way, but its measured value on main is ~3%, not 42%.
• Arm the per-row request timeout only when the decode suspends #275 implements this against main, so the number a reviewer reproduces there is ~208 ns/row / ~2.6–3.1% — correctly stated in that PR.

Attribution caveat

The size→cost mechanism is directionally confirmed (a padded always-ready inner future costs 51.7 ns at 8 B rising monotonically to 307.1 ns at 8176 B) but does not fully account for the absolutes — synthetic POD futures understate real state machines by 2.2×–5×, presumably drop glue and inlining differences. The per-row figures are measured; the attribution to the move specifically is partial and shouldn't be read as established.

Retitling to carry the corrected figures. Original title for the record: "Per-row tokio::time::timeout costs ~42% of row-decode time; cancellation wrapper adds ~5%".

[saurabh500]

Correction: the post-#252 figure was ~35%; it is ~15%. Measured, not estimated.

I set this issue's title to "~3% on main, ~35% after #252" earlier today. The ~35% was inflated ~2.4× by machine drift. The corrected figure is ~15% (~250 ns/row). Retitling accordingly.

Why the old number was wrong

The ~35% came from a mode ladder that runs every pass of one mode before moving to the next:

for mode in [Bare, CancelNone, CancelSome, CancelSomeTimeout, ...] {
    warmup(mode);
    measure(mode);   // all 9 passes, then next mode
}

Any drift across the sequence therefore lands entirely on mode order. The ladder ran timeout last, so monotonic upward drift inflates exactly the arm I was pricing.

I had defended the number on the grounds that the run showed an impossible ordering (cancel_some finishing 0.294 ms faster than cancel_none), and argued the true effect (12.336 ms) was 42× larger than that tell. That argument is invalid. The impossible ordering is evidence that drift exists; it is not a bound on its magnitude. Monotonic drift produces no impossible ordering at all and can be arbitrarily larger. Credit to the reviewer on #275 for naming this precisely.

The corrected instrument

run_paired_wrapper_ab (decode_bench.rs, commit dab45944) interleaves the two arms and takes the within-pair difference as the sample, so drift shared by both arms cancels regardless of its shape. Ordering alternates ABBA so within-pair bias cancels across pairs.

41 pairs per process, 5 independent processes, poc_row 48-col × 20 000 rows, ContiguousRowWriter, release:

process	ns/row	% of cancel_some	pairs with timeout arm slower
1	253.9	15.19%	40/41
2	247.7	14.58%	41/41
3	278.3	16.65%	39/41
4	244.9	14.65%	41/41
5	298.1	17.86%	41/41
median	253.9	15.19%	202/205

The effect is real — 202 of 205 pairs show it, which no plausible drift model produces — but it is ~15%, not ~35%.

This reconciles the two branches, and the reviewer's invariant was right

The reviewer on #275 held that the absolute per-row saving must be consistent across harnesses. Their paired measurement on main gave ~208 ns/row; my ladder gave ~617 ns/row. I argued the invariant needed a qualifier — "across harnesses measuring the same code" — because #[async_trait] on TdsPacketReader (#252) makes the wrapped future 2360 B on main and 4088 B here, and tokio::time::timeout takes its future by value.

The mechanism is real. It just does not account for a 3.0× gap. With the instrument repaired:

	main	this branch (has #252)
wrapped future	2360 B	4088 B
eager timeout cost	~208 ns/row	~254 ns/row
row decode without it	~6570 ns/row	~1670 ns/row
as % of row	~3.2%	~15.2%

Absolutes now differ by 1.22× against a 1.73× future-size ratio — and the synthetic size→cost ladder predicts 1.34× for that step. Same regime. The old 3.0× was my measurement error wearing a real mechanism as cover.

So the percentage rises because the denominator shrinks 3.9×, not because the absolute cost triples. That is a cleaner claim than the one it replaces, and it leaves the headline intact: this optimisation is worth roughly 5× more after #252 than before it.

What is unchanged

• The PR that implements this (Arm the per-row request timeout only when the decode suspends #275) targets main, so the figure a reviewer reproduces there is ~3%. That number came from a paired design and is unaffected.
• The lazy arms were measured only on the unpaired ladder, so I am not quoting a recovery fraction here.
• Convert TdsPacketReader to RPITIT to remove per-read boxing (row decode perf, axis: dispatch) #252 remains the dominant item at 2.60× on its own (155.6 → 58.8 ms, same-harness, per-commit), unaffected by any of this.