Sub-issue of #247 (axis: Precomputation). Flagged during review of #275.
Problem statement
get_next_row_into (mssql-tds/src/connection/tds_client.rs:3631) and next_row_cursor (:3696) take an unconditional Instant::now() before every per-row transport call:
let start = Instant::now();
let result = self.transport.receive_row_into(...).await?;
self.update_remaining_timeout(start);
update_remaining_timeout (:601-610) only consumes start inside remaining_request_timeout.map(..):
fn update_remaining_timeout(&mut self, start: Instant) {
self.remaining_request_timeout = self.remaining_request_timeout.map(|t| {
let elapsed = start.elapsed();
...
});
}
So when no command timeout is configured (remaining_request_timeout == None), the clock read at the top of the loop is pure overhead: its value is never observed. That is on exactly the per-row hot path #275 optimized, and the JS and CLI consumers commonly run with no timeout set.
When a budget is set this is two clock reads per row (Instant::now() plus start.elapsed()), which is inherent to the decrementing-budget model and not the target here.
Proposed solution
Skip the clock read when there is no budget: read Instant::now() only when remaining_request_timeout.is_some() and pass Option<Instant> into update_remaining_timeout, or hoist the is_some() check to the call site.
Same shape as #275: do not pay for timeout bookkeeping when no timeout is in play.
Other sites worth checking for the same pattern: :3532 (read_active_plp_chunk), :3799 and :3908 (resume_row_into), :3880 (drain_active_plp), :2371 (row discard loop).
Affected crate
mssql-tds
Alternatives considered
Cache one Instant per result set. Rejected. The budget deliberately charges only transport time, not caller think-time between row fetches, so hoisting a wall-clock deadline would change the timeout model. See the discussion in #271.
Leave it as is. Instant::now() is cheap, so this is likely a small win. It should be measured, not assumed.
Additional context
Not benchmarked. Explicitly out of scope for #275. Use the paired ABBA methodology from #271 to confirm before and after rather than assuming a win.
Sub-issue of #247 (axis: Precomputation). Flagged during review of #275.
Problem statement
get_next_row_into(mssql-tds/src/connection/tds_client.rs:3631) andnext_row_cursor(:3696) take an unconditionalInstant::now()before every per-row transport call:update_remaining_timeout(:601-610) only consumesstartinsideremaining_request_timeout.map(..):So when no command timeout is configured (
remaining_request_timeout == None), the clock read at the top of the loop is pure overhead: its value is never observed. That is on exactly the per-row hot path #275 optimized, and the JS and CLI consumers commonly run with no timeout set.When a budget is set this is two clock reads per row (
Instant::now()plusstart.elapsed()), which is inherent to the decrementing-budget model and not the target here.Proposed solution
Skip the clock read when there is no budget: read
Instant::now()only whenremaining_request_timeout.is_some()and passOption<Instant>intoupdate_remaining_timeout, or hoist theis_some()check to the call site.Same shape as #275: do not pay for timeout bookkeeping when no timeout is in play.
Other sites worth checking for the same pattern:
:3532(read_active_plp_chunk),:3799and:3908(resume_row_into),:3880(drain_active_plp),:2371(row discard loop).Affected crate
mssql-tds
Alternatives considered
Cache one
Instantper result set. Rejected. The budget deliberately charges only transport time, not caller think-time between row fetches, so hoisting a wall-clock deadline would change the timeout model. See the discussion in #271.Leave it as is.
Instant::now()is cheap, so this is likely a small win. It should be measured, not assumed.Additional context
Not benchmarked. Explicitly out of scope for #275. Use the paired ABBA methodology from #271 to confirm before and after rather than assuming a win.