decode: bulk path for packed fixed-width payloads#314
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Packed fixed32/sfixed32/float/fixed64/sfixed64/double payloads previously
decoded one element at a time: a remaining-bytes check, a generic-Buf
fixed-width read, and a Vec push per element. The element count of a
fixed-width packed payload is fully determined by the byte length, so the
whole payload can be validated, reserved, and converted in one pass.
This adds six bulk extenders (buffa::types::extend_packed_{float,fixed32,
sfixed32,double,fixed64,sfixed64}) that validate payload alignment up
front, reserve the exact element count, and convert via chunks_exact +
from_le_bytes -- endian-correct everywhere and a bulk copy on
little-endian targets. Codegen routes packed fixed-width arms through
them on the view path (always slice-contiguous) and on the owned path
when the payload is contiguous in the current chunk and the field uses
the default Vec; fragmented buffers and custom list representations keep
the per-element loop, as do all varint-family payloads (their element
count is not derivable from the byte length).
Semantics: accept/reject behavior is unchanged (a misaligned payload
still fails the field with DecodeError::UnexpectedEof; differential
tests in buffa/src/types.rs pin bulk-vs-per-element parity across valid,
truncated, and empty payloads). One observable difference on the bulk
paths: a malformed payload now leaves the field unmodified instead of
partially extended before the error; per-element fallback paths are
unchanged.
Motivation: long packed-double arrays (hundreds to thousands of elements
per field, e.g. per-element score vectors) are decode-dominated by the
per-element overhead. On such shapes the bulk path measures at multiples
of the per-element loop in isolated benchmarks; short arrays (the
PackedTile bench shapes) are unaffected within noise because per-field
overhead dominates there.
Conformance: identical results to main on the full suite (std, via-view,
via-lazy; binary/JSON/text), same runner, same known-failure sets.
Workspace test suite green; the two codegen intent tests that pinned the
per-element shape for fixed-width types now pin the bulk-extender shape.
2a1e350 to
8f208a5
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
[claude code] Reviewed for correctness and benchmarked on dedicated bare metal. The correctness is sound and the owned-path win is real — I want two test additions and a rebase before it lands, and there's a measurement finding you'll want. The owned-path gain reproducesThree-way compare on a spot
Both controls are datasets your patch provably cannot touch — mesh has no packed fields at all, packed_signed is varint-only — and both sit under 0.5%. So the −6.58% is a real effect, close to your reported −11%. One methodology note that cost me a run, and would have cost you one too. My first attempt showed The view-path numbers are unusable, and the suite can't show your real winTwo separate problems, neither your fault:
Correctness: no Critical or HighVerified rather than assumed: the accept/reject set is identical to the per-element path for every input; double-occurrence still concatenates ( The reserve is not a #287-class amplification surface — I checked specifically, since that's the bug we just fixed twice this week. Two asks before merge
Optional hardening, your call: a Nice work — the differential tests are load-bearing (I mutation-tested them: disabling the width check fails four), and tightening the existing fixed-width test to compare error variants was a real improvement. |
|
[claude code] Follow-up: I took you up on the long-array benchmark rather than asking you to write it — #318 adds Re-ran the same three-way metal compare with the new bench included (base = this branch's merge-base, layout-normalized, spot
−24.66% against −8.59% on Two details worth calling out:
The message is a columnar batch — one field per column, every row's value contiguous — because that's what analytics and telemetry pipelines actually send, so it earns its place in the suite beyond settling this PR. Columns are chosen so each lands in a distinct decode path, verified against the wire bytes: dictionary strings, 1.00 B/elem indices (the 1-2 byte varint case), 9.00 B/elem timestamps (the wide case), 8.00 B/elem doubles and 4.00 B/elem floats. Nothing changes about the asks — the rebase onto current main (eleven PRs landed today), the fragmented- |
…ences The owned decoder picks between the bulk copy and the per-element loop on whether the payload sits in the current chunk, and it is generic over Buf, so which branch runs is the caller's buffer layout rather than anything on the wire. Every existing test decodes from a contiguous slice, so the fallback the correctness argument rests on had no coverage at all. Decode the same message from a two-chunk buffer split at every byte, which puts the boundary inside the packed payloads and mid-element, and require it to match the contiguous decode. Forcing the bulk path unconditionally makes that test panic reading past the first chunk, so it does reach the branch rather than passing by luck. Also pin that a packed fixed-width field appearing twice concatenates. The existing occurrence test covers varints only, and the bulk path reserves per occurrence, so nothing showed the reserve was additive rather than truncating.
iainmcgin
left a comment
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] Approving. I've merged current main and added the two tests I asked for rather than sending you round again — 15eca83 (merge) and 021b876 (tests).
M1 — the fragmented-Buf fallback. The owned decoder chooses between the bulk copy and the per-element loop on buf.chunk().len() >= len, and it's generic over Buf, so which branch runs is the caller's buffer layout rather than anything on the wire. Every existing test decodes from a contiguous slice, so the branch your correctness argument depends on had no coverage. The new test decodes the same message from a two-chunk buffer split at every byte, which puts the boundary inside the packed payloads and mid-element, and requires it to match the contiguous decode.
I checked the test isn't decorative: forcing the routing to if true (always bulk) makes it panic with range end index 12 out of range for slice of length 1, so it genuinely reaches the fallback rather than passing by luck.
L1 — double-occurrence on the bulk fixed-width path. The existing repeated_mixed_packed_and_unpacked_on_wire covers varints only, and your bulk path reserves per occurrence, so nothing pinned that the reserve is additive rather than truncating. A packed double field appearing twice now has to concatenate.
Gates on the merged branch: cargo fmt --check, clippy --workspace --all-targets -D warnings, cargo test --workspace (48 suites) all clean.
Left the optional hardening from my review alone — the const width assert, the endianness-pinning assertion, and the comment about chunking affecting malformed-input side effects are all reasonable follow-ups but none of them blocks this.
For the record, the numbers this lands on: −24.66% on column_batch/decode (the new 1024-element columnar bench in #318), against −8.59% on packed_tile's short arrays, with mesh controls quiet at −2.13%/−3.00% and packed_signed at −0.06% — exactly the zero a fixed-width patch should show on a varint-only message. Your original −11% was, if anything, modest about it.
Thanks for this, and for the unusually careful PR body — the routing rules and the documented semantic difference were both accurate, which made reviewing it a matter of verification rather than discovery.
|
[claude code] Merged with an admin override past a red The CLA requirement is satisfied; the check could not observe it. The action failed with: It is getting an HTML error page where it expects JSON — GitHub's API was intermittently serving those tonight (it broke a couple of my own Verified both commit authors independently of the action:
Every other check is green on the rerun, including conformance (3m28s) and Thanks again @taoroalin — the numbers on the new columnar bench (#318) put this at −24.66% on 1024-element columns, three times what our short-array benchmarks were showing. |
2 participants
What: packed fixed32/sfixed32/float/fixed64/sfixed64/double payloads decode in one bulk call (six new
buffa::types::extend_packed_*helpers: upfront alignment check, exact reserve,chunks_exact+from_le_bytesconversion) instead of a per-element remaining-check/read/push loop. Codegen routes the view path through them always (views are slice-contiguous), and the owned path when the payload is contiguous in the current chunk and the field uses the defaultVec; fragmented buffers, custom list representations, and varint-family payloads are untouched by this patch.Why: for long packed arrays the per-element bounds check and generic-Buf read dominate;
chunks_exact+from_le_bytescompiles to a bulk copy on little-endian targets. Short arrays (PackedTile's 4-8 elements) sit under per-field overhead, which is consistent with the existing comment that the plain decoders are optimal in packed loops — the regime the existing benchmarks cover. The workload that motivated this has packed-double arrays of hundreds to thousands of elements per field.Measured (shared-container box, so ratios not absolutes; noise floor ±10% per cell on our box, worse on the smallest benches; per-row minima across repeated runs):
Semantics: accept/reject identical (misaligned payload still fails with
DecodeError::UnexpectedEof); one documented difference — on the bulk paths a malformed payload leaves the field unmodified rather than partially extended (changelog fragment included). Differential tests pin bulk-vs-per-element value AND error-variant parity.Validation: full conformance suite byte-identical to main (std, via-view, via-lazy; same runner, same known-failure lists); workspace tests green; clippy/fmt clean; the two codegen intent tests updated to pin the new shape. Before opening this PR I re-verified on this exact branch: 2477 workspace tests pass,
cargo fmt --checkandcargo clippy --all-targetsclean. The conformance run above was done during development; the machine this PR was pushed from has protoc 29.6 (< v30 needed for the conformance build), so CI is the authoritative conformance signal here.Authorship/process note: developed and validated by a Claude agent session at Anthropic, which can't push to this repo; I'm opening the PR on its behalf. Happy to iterate on review feedback.
Follow-up #315 stacks on this one, applying the same pattern to packed plain-varint payloads.