Converge decode onto decode_into via CaptureWriter - #269
Converge decode onto decode_into via CaptureWriter#269Saurabh Singh (saurabh500) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Precomputes column decode specifications for the TDS row pipeline, addressing #249 under the #247 performance work.
Changes:
- Adds and caches
ColumnDecodeSpecplans fromCOLMETADATA. - Unifies value and writer decoding through one planned dispatch.
- Expands decode-shape and regression coverage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
mssql-tds/src/datatypes.rs |
Registers the decode-plan module. |
mssql-tds/src/datatypes/decode_spec.rs |
Defines planning, classification, errors, and tests. |
mssql-tds/src/datatypes/decoder.rs |
Replaces duplicate type switches with planned decoding. |
mssql-tds/src/datatypes/row_writer.rs |
Adds single-value capture support. |
mssql-tds/src/datatypes/sql_string.rs |
Makes encodings copyable and safely resolves collation. |
mssql-tds/src/io/token_stream.rs |
Uses cached plans in row decoding and skipping. |
mssql-tds/src/token/tokens.rs |
Stores the eager per-column plan. |
mssql-tds/src/token/parsers/colmetadata_parser.rs |
Builds planned metadata tokens. |
mssql-tds/src/token/parsers/row_parser.rs |
Updates test metadata construction. |
mssql-tds/src/token/parsers/nbcrow_parser.rs |
Updates test metadata construction. |
mssql-tds/src/connection/tds_client.rs |
Updates test metadata construction. |
mssql-tds/src/connection/metadata_retriever.rs |
Updates test metadata construction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| match (spec.encrypted, decryptor) { | ||
| (true, Some(dec)) => { | ||
| let value = decrypt_encrypted_column(decoder, reader, meta, dec).await?; |
Provisional benchmark: this change is a regression on the #238 schemaMeasured as a provisional number against the unmerged dispatch branch ( MethodIn-crate harness ( Baseline = Results
Best-observed minima, which are the least noise-contaminated statistic:
On WhyThe premise of #249 was that the per-cell So on a schema that is 81 % Per cell the plan-driven path adds a bounds-checked load and a 24-byte copy of The RecommendationDo not merge as-is. The correctness and convergence work stands on its own — one type switch instead of two, no fallback variant, encryption kept out of the enum — but #249 was justified as a performance change, and as measured it is not one. Two concrete directions, if this is worth another pass:
Both attack the added dispatch cost rather than the hoisting, which is where the measurement points. Numbers are provisional: they are taken against an unmerged draft. They should be re-taken once #264 merges. |
Benchmark round 2 — after shrinking
|
| type | before | after |
|---|---|---|
ColumnSpec |
24 B | 6 B |
ColumnDecodeSpec |
20 B | 4 B |
VarU8Kind |
— | 3 B |
VarU16Kind |
— | 4 B |
Method
Unchanged from round 1, plus a second sink axis. In-crate, driving receive_row_into_internal over a pre-built buffer; 60 000 rows x 48 columns = 2.88M cells per pass; median of 9 after 3 warmups; --release; 4 independent runs per cell of the table. Baseline is dispatch tip cfa0dfe7; both branches carry an identical bench module.
discard is DiscardRowWriter (cheapest possible sink, maximises decode's share). materialize is DefaultRowWriter, which builds ColumnValues per cell and hands the row off — the shape the public row API and the JS binding actually use.
Results
Median of the four per-run medians:
| schema | sink | baseline | +#249 | Δ |
|---|---|---|---|---|
int_varchar (39xINT + 9xVARCHAR(6)) |
discard | 90.36 ms | 100.41 ms | +11.1 % |
int_varchar |
materialize | 163.73 ms | 183.18 ms | +11.9 % |
mixed_shapes (INT/VARCHAR/DECIMAL/TIME/DATETIME2/GUID) |
discard | 174.97 ms | 168.70 ms | −3.6 % |
mixed_shapes |
materialize | 255.74 ms | 252.12 ms | −1.4 % |
Least-disturbed run (min of the four per-run minima), which is the more robust statistic for a CPU-bound loop:
| schema | sink | baseline | +#249 | Δ |
|---|---|---|---|---|
int_varchar |
discard | 87.69 ms | 97.35 ms | +11.0 % |
int_varchar |
materialize | 154.66 ms | 167.17 ms | +8.1 % |
mixed_shapes |
discard | 166.60 ms | 159.68 ms | −4.2 % |
mixed_shapes |
materialize | 242.29 ms | 239.91 ms | −1.0 % |
Confidence. Only int_varchar/discard has non-overlapping per-run distributions across all four runs (baseline 89.43–91.41, spec 98.62–109.56). The other three rows overlap, so treat mixed_shapes −3.6% as suggestive rather than established, and the materialize rows as directionally consistent but noisy.
Diagnostic: the remaining cost is not future size
Round 1 attributed the cost partly to the spec inflating the generated async state. That is now measured and refuted:
| future | baseline | +#249 |
|---|---|---|
decode_into |
928 B | 216 B (−77 %) |
receive_row_into_internal |
1328 B | 848 B (−36 %) |
The plan-driven path produces substantially smaller futures and is still slower on the INT-heavy schema. Neither the by-value copy nor async-state growth explains what is left.
Where that leaves the model
Modelling the change as a fixed per-cell cost X minus a classification saving Y:
int_varchar:Y ≈ 0(anIntNcell needs noget_encoding_type/is_plp/get_scale), so it measuresX≈ 3.5 ns/cell — down from 6.6 ns/cell before the shrink.mixed_shapes:Yis real; at −3.6% on a 62 ns/cell baseline,Y ≈ 5.7ns/cell.
Shrinking the spec 4x roughly halved X, and mixed_shapes moved from −0.8% to −3.6% exactly as that model predicts. If X could be driven to zero, mixed_shapes extrapolates to about −9%.
Two candidates remain for the residual X, and they are not distinguished by this data:
- An added dependent load. The spec now comes from a second array that must be loaded before the branch can resolve, where the baseline's discriminant was a field of the
ColumnMetadatait was already walking. - Two-level dispatch.
ColumnDecodeSpec::Fixed(FixedKind::I32)is a nested pattern, so two dependent switches on the arm that serves 39 of 48 cells.
One observation argues for (1) over (2): mixed_shapes exercises far more distinct arms and improved, which is the opposite of what branch-predictor pressure would produce.
Status
Still do not merge as a performance change on integer-heavy schemas. The regression on the #238 shape is real and reproducible at +11%, roughly halved from +19.7% but not eliminated. The shape-diverse schema is now modestly positive.
Distinguishing the two remaining hypotheses requires flattening the spec enum, which is a design change rather than a tuning change, so it is not being taken unilaterally. Branches: dev/saurabh/bench-spec-on-dispatch and dev/saurabh/bench-dispatch-baseline.
Benchmark round 3 — retiring the metadata walk (provisional, on top of unmerged #264)Round 2 left the plan being consumed alongside the column metadata: It did not. Reporting the numbers. What changed (
|
| schema | sink | baseline | +#249 | Δ | per-run distributions |
|---|---|---|---|---|---|
int_varchar (39×INT + 9×VARCHAR(6)) |
discard | 87.42 ms | 98.57 ms | +12.8 % | separated (84.92–89.12 vs 96.03–110.46) |
int_varchar |
materialize | 154.13 ms | 163.72 ms | +6.2 % | separated (153.54–154.34 vs 160.12–165.38) |
mixed_shapes |
discard | 163.23 ms | 160.61 ms | −1.6 % | overlapping |
mixed_shapes |
materialize | 235.05 ms | 237.17 ms | +0.9 % | overlapping |
Both int_varchar cells are non-overlapping regressions. Both mixed_shapes cells are inside noise.
Round-2 figures were +11.1 % / +11.9 % / −3.6 % / −1.4 %, but the absolute baselines moved between rounds (mixed_shapes/discard baseline 174.97 → 163.23 ms, ~7 %), so cross-round comparison is not reliable and I am not claiming a trend from it. The within-round conclusion is what stands: retiring the metadata walk did not recover the regression.
Cause, by elimination
Three candidates have now been tested and refuted by measurement:
| candidate | test | result |
|---|---|---|
| 24-byte by-value spec copy | shrank ColumnSpec 24 B → 6 B |
regression persisted |
| async state growth | measured generated future sizes: decode_into 928 → 216 B, receive_row_into_internal 1328 → 848 B |
futures shrank 77 % / 36 % and it was still slower |
| second 640-byte metadata stream | removed metadata from the hot loop entirely | regression persisted (this round) |
What remains is the extra dispatch level. Baseline dispatches once on metadata.data_type and then on the length byte. The spec path dispatches on ColumnDecodeSpec, then on the inner kind, then on the length byte — three levels where baseline has two. The residual is ≈ 3.9 ns/cell (~12 cycles), which is the right order for an additional indirect branch.
This is consistent with the schema split rather than contradicted by it: model the plan as adding a fixed per-cell dispatch cost X and removing a classification cost Y. For INT cells Y ≈ 0, so int_varchar measures X alone. For mixed_shapes, Y (per-cell get_encoding_type / is_plp / get_scale) is real and roughly cancels X.
Status
This is the third and final measurement round agreed for this axis. The numbers are provisional in the sense that they are taken on top of unmerged #264, not on main.
Branches, both pushed and reproducible:
dev/saurabh/bench-spec-on-dispatch@5d7f1e60dev/saurabh/bench-dispatch-baseline@5fcde533
The benchmark is byte-identical on both branches except metadata_token(), which must construct the token differently because ColMetadataToken::new only exists on the spec side.
Validation on 5d7f1e60: cargo bfmt clean, cargo bclippy clean, cargo nextest -p mssql-tds --lib 1734 run / 1727 passed / 7 failed (the pre-existing expired-certificate fixtures, unrelated), RUSTFLAGS='--cfg fuzzing' cargo check -p mssql-tds --lib clean.
Row decoding maintained two hand-synced ~40-arm switches over TdsDataType: decode_into, which drives RowWriter, and decode, which builds ColumnValues. A divergence between them is silently wrong data rather than a compile error. Collapse them: decode is now defined as decode_into plus a CaptureWriter that records the written value. This deletes GenericDecoder::decode's ~320-line switch and StringDecoder::decode's ~80-line near-duplicate. decode_into previously delegated eight types (Xml, Json, Udt, Image, Vector, SsVariant, Decimal, Numeric) back to decode via a fallback arm. Those now have real arms, so the residual _ => arm is terminal UnimplementedFeature, matching what decode already did for the same types. Extract read_long_len_bytes, removing a third copy of the LONGLEN wire format that was duplicated across the Image arm, decode_string_into and StringDecoder::decode. This unifies two oversize error messages into one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d3a0066b-1b8a-4581-afc9-ec2b9d3034fb
e638bee to
2b22354
Compare
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-tds/src/datatypes/decoder.rsmssql-tds/src/datatypes/row_writer.rs🔗 Quick Links |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
mssql-tds/src/datatypes/row_writer.rs:276
- Treating “no value written” as SQL
NULLhides a violated decoder contract. NULL paths already callwrite_null, so a successful decode with no write is an implementation error; similarly, repeated writes currently overwrite the first value. Track the number of writes and return an error unless exactly one value was captured, then propagate that result from bothdecodeimplementations. Otherwise a future missing/duplicate writer call can silently produce wrong data—the failure mode this convergence is intended to remove.
/// Takes the captured value, or `Null` if the decoder wrote nothing.
pub(crate) fn into_value(self) -> ColumnValues {
self.value.unwrap_or(ColumnValues::Null)
mssql-tds/src/datatypes/decoder.rs:1344
- These arms no longer fall back to
decode(), but thedecode_intodocumentation at lines 1068–1071 still says all rare types do. Update that documentation to describe the single-switch implementation and, if useful, distinguish the helpers that temporarily materializeColumnValues.
// === Rare shapes: boxed so their locals stay out of the hot path's
// future, but still handled here rather than in a second switch. ===
CaptureWriter::into_value returned NULL when a decode_into arm wrote nothing, which would turn a missing writer call into silently wrong data - the failure mode this convergence exists to remove. write_null already records Some(Null), so an empty slot is unambiguously a decoder bug and is now reported as ImplementationError. Route the 24 setters through a checked helper so a second write for the same column trips a debug assertion rather than overwriting the first. Also correct decode_into's doc comment, which still described the rare types as falling back to decode(). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d3a0066b-1b8a-4581-afc9-ec2b9d3034fb
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d3a0066b-1b8a-4581-afc9-ec2b9d3034fb
Final convergence resultClosing after the required remeasurement on top of #264 and one pre-committed MeasurementsAll rounds used the same 60,000-row × 48-column in-crate harness, with 3 warmups Absolute baselines drifted between sessions ( Before #264: no regression; three cells overlapped and the separated cell
After #264 (
The same harness independently corroborated #264's headline: One cold-partition attempt: the code-size mechanism below was confirmed, so
That is a material recovery, but it fails the pre-committed Mechanisms testedFuture size — refuted across three benchmark rounds. In #249 round 2, Inlining — did not recover the loss. A bounded Generated code size — confirmed as a partial cause, then shown insufficient.
The cold partition reduced each real-writer Total decode footprint and the extra For the concrete
The third copy is the Two caveats are load-bearing. Cold code that never executes should not consume The reusable conclusion is: future size is not a proxy for row-decode Cheaper successorThe risk this PR addressed is silent drift between The EvidenceThe session artifact archive retains:
The future-size/codegen postscript is also recorded on No experimental code was ported. The branch remains clean at Successor filed: #289 — Add exhaustive Tracker: #247 — carries the consolidated record of which metrics predicted decode throughput on this path and which did not, and the standing requirement to measure on top of merged #264 rather than pre-#264 |
What
Row decoding maintained two hand-synced switches over
TdsDataType:GenericDecoder::decode_into— 33 arms, drivesRowWriterGenericDecoder::decode— ~320 lines, buildsColumnValuesplus an ~80-line near-duplicate pair in
StringDecoder. The two produce the samevalues by convention only. A divergence between them is silently wrong data,
not a compile error.
This PR collapses them.
decodeis now defined asdecode_intoplus aCaptureWriterthat records whatever was written:There is now one type switch. Adding a
TdsDataTyperequires touching oneplace, and the two decode sites cannot drift.
Changes
row_writer.rs— addsCaptureWriter, aRowWriterthat stores the singlevalue written to it and hands it back as
ColumnValues. This is the mechanismthat makes the collapse possible.
decoder.rsdecode_into's_ =>arm previously delegated eight types back todecode:Xml,Json,Udt,Image,Vector,SsVariant,Decimal,Numeric.Each now has a real arm.
DecimalandNumericget an explicit rejectionarm; the residual
_ =>is terminalUnimplementedFeature, which is exactlywhat
decodealready did for the same set.decode_intonow names 41 of46
TdsDataTypevariants; the terminal arm covers exactly five, none ofwhich is a supported result-column type:
Void0x1FNULLTYPEis never emitted by the serverVarBinary0x25BigVarBinaryBinary0x2DBigBinarySqlTable0xF3None0x00TdsDataType::try_fromAll five are rejected by
read_type_infobefore row decode, or cannot beparsed at all. Full enumeration on Precompute a per-column decode plan (
ColumnDecodeSpec) and converge all decode sites on it #249.Deletes
GenericDecoder::decode's ~320-line switch.Deletes
StringDecoder::decode's ~80-line near-duplicate body.Deletes
read_intn, the now-deadColumnValuestwin ofdecode_into'sIntNarm.Extracts
read_long_len_bytes, removing a third copy of the LONGLEN wireformat that was duplicated across the
Imagearm,decode_string_into, andStringDecoder::decode.Net +4 lines against current
main, including 15 new tests.Behaviour
Behaviour-preserving, with one cosmetic exception noted below.
The eight newly-inlined arms call the same helpers
decodecalled, with thesame argument order and the same error text. Empty-payload semantics are
preserved and now pinned by tests:
imagetreats an empty payload as NULL,text/ntexttreat it as an empty string.One cosmetic change. Unifying the LONGLEN reader merged two oversize-length
error messages —
"Image length ..."and"Text data length ..."— into asingle
"LOB data length {length} exceeds maximum allowed size of {MAX} bytes".No test asserted either original string (verified by grep). Both were
ProtocolError, so the error type is unchanged; only the message text differs.Unreachable-by-construction tripwire.
CaptureWriter::into_valuenow returnsErr(ImplementationError)rather thanNULLif adecode_intoarm returnsOkwithout writing. Every arm either writes exactly once or returns an error, so
this is unreachable today and changes no observable behaviour — but reporting a
missing write as
NULLwould be precisely the silent wrong-data failure this PRexists to remove, so the replacement should not reintroduce a smaller version of
it. A SQL
NULLis written explicitly viawrite_null, so the two cases areunambiguously distinguishable. A second write for the same column trips a debug
assertion. (Raised by Copilot review.)
Tests
15 new tests covering every newly-inlined arm and the terminal residual:
decode_into_xml_plp,decode_into_json_plp,decode_into_udt_plp,decode_into_xml_plp_null,decode_into_json_rejects_non_plp_metadata,decode_into_image_value,decode_into_image_null_text_pointer,decode_into_image_empty_payload_is_null,decode_into_ssvariant,decode_into_fixed_decimal_is_unimplemented,decode_into_fixed_numeric_is_unimplemented,decode_into_unknown_type_is_unimplemented,capture_writer_returns_written_value,capture_writer_distinguishes_written_null_from_no_write,capture_writer_errors_when_nothing_was_written.They use the existing
assert_decode_equivalenceharness, which assertsdecode_intoanddecodeconsume identical bytes and produce identical values— the property this PR makes structural rather than aspirational.
Validation
cargo bfmtcargo bclippycargo nextest -p mssql-tds --libcargo btest(workspace, before merging #264)RUSTFLAGS='--cfg fuzzing' cargo check -p mssql-tds --lib.\scripts\bfmt.ps1(incl.mssql-py-core).\scripts\bclippy.ps1(incl.mssql-py-core)The 7 current lib failures are the pre-existing expired-certificate fixtures
(
certificate_validator×4,win_tls::validate×3) — unrelated and failing onmain. The earlier 364btestfailures are all intests/integration binariesfailing at TLS handshake (
SEC_E_WRONG_PRINCIPAL); they need a live SQL Server.Zero
datatypes::orio::unit-test failures.Performance
Post-#264 remeasurement found a regression. After merging current
main(
#264at7a9c0b93), the same in-crate harness was rerun against that commitand this PR at
de42362c: 60,000 rows × 48 columns, 3 warmups, 9 measuredpasses, four alternating baseline/treatment runs. Each figure is the median of
the four per-run medians.
mainint_varchar/ discardint_varchar/ materializemixed_shapes/ discardmixed_shapes/ materializeThis invalidates the pre-merge “no change” result and keeps the PR in draft
pending disposition. A bounded
#[inline(always)]experiment ondecodeanddecode_intodid not recover the integer-heavy regression. Future-state growthis also ruled out: convergence shrinks
receive_row_into_internalfrom 1,392 Bto 872 B and
drive_row_columnsfrom 1,232 B to 712 B. The cause is not yetproven.
The benchmark harness is deliberately not committed to this branch.
History
This PR originally implemented #249 — a precomputed per-column
ColumnDecodeSpec. That performance premise was tested and refuted acrossthree rounds of measurement; the write-up is on #249, which is closed as
tested-and-refuted.
The convergence never depended on the precomputation, so it was kept and the
plan was dropped. What remains is the part that carried the value: one type
switch instead of two, justified on correctness and maintainability alone.
Relates to #247, #249.