decode: slice-specialized loop for packed plain-varint payloads

[taoroalin]

Stacks on #314 (shares the extender pattern and the codegen routing) — its commit is included here as the first commit; only the second commit is new to this PR.

What: packed int32/int64/uint32/uint64/sint32/sint64/bool payloads decode through seven new bulk extenders built on one driver (encoding::for_each_packed_varint): one reserve up front (views keep their exact count_varints hint; owned decode keeps its byte-length upper bound — each path's pre-existing policy), then decode over a position index — 1-2-byte varints (ids, tokens, enum-sized values) inline with no per-element chunk or remaining checks, longer elements through the existing unrolled slice decoder. Same routing rules as the fixed-width PR (view always; owned when contiguous + default Vec; enums keep the loop for unknown-value routing).

Why: decode_varint_packed already removed the out-of-line call per element; what remains per element is the chunk()/len dispatch, the slice-decoder precondition re-check, and advance() bookkeeping — all loop invariants for a payload whose final byte has a clear continuation bit (every well-formed payload). Hoisting them is the last step the existing design points at.

Measured (same box/method caveats as the fixed-width PR): on varint-dense shapes our calibrated internal workload shows +6-12% on top of the fixed-width patch for frames carrying packed uint32 arrays, rising to +26% on the owned path for a frame dominated by a 4,000-element packed-uint32 array. On the native suite in this repo, packed_tile decode improves a further -25% from this patch (777 -> 579 µs on our box, interleaved A/B at ~3% resolution; -31.6% total across the two patches vs base): PackedTile's cells each carry a packed-uint32 geometry field (12-20 elements x 64 cells) plus a packed-uint64 checksums field, which this patch's extenders decode directly. packed_signed/mesh rows moved within our box's noise floor — a long-array variant of those benches would make the per-element effect visible (happy to contribute one).

Semantics: unchanged, including malformed payloads — an unterminated trailing element routes the whole payload to the per-element decoder, preserving exact error and partial-extension behavior; overlong-but-terminated and 10-byte/VarintTooLong cases decode identically. Differential tests pin value and error-variant parity across valid, truncated, overlong, and unterminated payloads.

Validation: conformance byte-identical to main across std/via-view/via-lazy (this patch activates the owned-path routing for descriptor.proto's packed int32 fields, so the checked-in buffa-descriptor generated code is regenerated here; the regen is mechanical); workspace tests green; clippy/fmt clean. Before opening this PR I re-verified on this exact branch: 2479 workspace tests pass, cargo fmt --check and cargo clippy --all-targets clean. The conformance run 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.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[taoroalin]

I have read the CLA Document and I hereby sign the CLA

[iainmcgin]

[claude code] Reviewed for correctness and benchmarked on dedicated bare metal. No Critical, High, or Medium findings — this is careful work, and the riskiest part of it holds up under attack.

The claim I tried hardest to break

"1-2 byte varints inline with no per-element chunk or remaining checks" is the sort of sentence that usually hides an over-read on the last element. It doesn't here. payload[i+1] is reached only when payload[i] >= 0x80, and the caller's precondition — final byte < 0x80 — guarantees byte i isn't the last, so the index is always in bounds. The precondition is airtight rather than incidental: for_each_packed_varint is pub(crate) with exactly one caller, and that caller only enters the fast path in the last < 0x80 arm, with a debug_assert! backing it. No unsafe, no unchecked indexing. An exhaustive brute force over every byte string up to length 5 (from {0x00,0x01,0x7f,0x80,0x81,0xff}) found no panic and zero value-or-error divergence from the per-element decoder.

The enum check also passes, which was the one I most wanted verified: extend_packed_varint_fn_token returns None for TYPE_ENUM, so closed-enum packed fields keep the per-element loop on both paths and the unknown-value routing that #313 and #304 just landed is not bypassed. Had that gone the other way it would have silently undone a guard fix from the same release.

Your malformed-payload contract checks out exactly as written: dispatch on payload.last() routes unterminated tails to a literal per-element loop over the same decoder, while overlong-but-terminated and 10-byte/VarintTooLong payloads have a final byte < 0x80 and take the fast path into a byte-identical decoder body. Error variants and pre-error contents match by construction. The reserve asymmetry is also exactly what you described — views keep count_varints, owned keeps the byte-length bound — and I confirmed both match what main already did, so it's not a new amplification surface. zigzag and bool conversions mirror the scalar decoders precisely.

The measurement

Three-way compare on a spot c7i.metal-24xl, base = this stack's own merge-base:

benchmark	delta vs base
packed_signed/decode (your direct target: packed int32/int64)	−8.16%
packed_tile/decode (cumulative with #314)	−13.82%
mesh/decode (control — no packed fields at all)	+4.84%

The −8.16% on packed_signed is your patch hitting exactly what it aims at. Read it with the control in mind though: mesh/decode moved +4.84% on a dataset the patch cannot touch, so the owned-path noise on this build pair is ~±5% and the effect is above the band but not enormously so.

View-path rows are unusable (mesh/decode_view −26.04%, again a dataset with no packed fields), and the reason is structural: this stack is based pre-#312, and #312 — the only commit since your base touching the view decode path — restructures that loop entirely. Every view number here describes a shape that will never ship. Please rebase onto current main; ten PRs landed today.

If you re-measure, the bench crate does not wire in the repo's layout-normalization flags despite building at cgu=1/lto=true; pass them by hand or the controls swing ~9%:

RUSTFLAGS='-Cllvm-args=-align-all-nofallthru-blocks=6 -Cllvm-args=-align-loops=64' cargo bench --bench protobuf --no-run

One advisory

check_bulk_u32 asserts value parity only on Ok; on error it compares the error variant but not the partial out vector. Your commit message claims partial-extension behaviour is preserved, and it is — by construction, and the partial vector is discarded on failure anyway — so this is a documentation-vs-test gap rather than a defect. Asserting partial equality on the erroring cases would pin the claim you're making. Your call.

Since this stacks on #314, it lands behind it — I've asked there for a fragmented-Buf test and a bulk double-occurrence test, plus the long-array benchmark you offered. That last one matters most for this pair: the suite's longest fixed-width packed array is 8 floats, so it structurally cannot show the 64+-element win your own numbers report. I'd rather land these with a bench that demonstrates the point than on an out-of-tree workload.

[iainmcgin]

[claude code] Follow-up: #318 adds the long-array benchmark you offered to write — ColumnBatch, a columnar record batch with 1024-element packed columns. Your numbers hold up, and our suite was under-reporting this stack badly.

Same three-way metal compare, now with the new bench (base = this stack's merge-base, layout-normalized):

benchmark	#314 alone	this stack
column_batch/decode	−24.66%	−36.23%
column_batch/decode_view	−15.57%	−20.48%
packed_signed/decode (your direct target)	−0.06%	−7.62%
packed_tile/decode (4-20 elem)	−8.59%	−16.33%
mesh/decode (control)	−2.13%	+1.73%
mesh/decode_view (control)	−3.00%	−3.61%

The separation between the two patches is unusually clean. packed_signed is varint-only, so under #314 (fixed-width only) it reads −0.06% — exactly the zero it should be — and your commit moves it to −7.62%. Meanwhile column_batch carries both regimes: ~12 KB of its 22.9 KB payload is fixed-width (#314's) and ~10 KB is the varint columns (yours), and the cumulative −36.23% is close to the whole payload getting faster. Each patch hits precisely its own target.

Two of the columns exist for your patch specifically, and were verified against the wire bytes rather than assumed: symbol_ids at 1.00 B/elem is the 1-2 byte inline case your fast path is built for, and timestamps at 9.00 B/elem (absolute epoch nanos, not deltas) is the wide case that routes through the unrolled slice decoder. A delta encoding would have been the obvious "realistic" choice and would have collapsed both columns into the same regime, testing one path twice.

Also worth noting: every control is quiet this run (−2.13% to +1.73%), so unlike my earlier attempts — where mesh/decode_view swung −26% — the view numbers here are trustworthy, and they show a genuine −20.48%.

Asks are unchanged: the rebase (eleven PRs landed today), and #314's two test additions ahead of this one. The correctness review stands — no Critical, High, or Medium, and I could not construct an over-read against your fast-path bound.

[iainmcgin]

[claude code] Approving. #314 landed as d1db9a7, so I rebased this onto current main — it now shows a single commit (f7524b6, your varint work) instead of carrying its own copy of #314's, which the squash had turned into a duplicate. Verified #314's bulk fixed-width code and tests are present from main rather than replayed, and that its split-buffer test still passes on top of this change.

The correctness review stands unchanged: no Critical, High, or Medium. The two findings I most wanted to break both held —

• The fast-path bound is airtight. payload[i+1] is reached only when payload[i] >= 0x80, and the caller's precondition (final byte < 0x80) guarantees byte i isn't the last. Proven from the precondition and confirmed by brute force over every byte string up to length 5 from {0x00,0x01,0x7f,0x80,0x81,0xff} — no panic, zero divergence from the per-element decoder. No unsafe, no unchecked indexing.
• Enums correctly keep the per-element loop, so the closed-enum unknown-value routing that #313 and #304 landed earlier today is not bypassed. Had that gone the other way it would have silently undone a guard fix from the same release.

Also confirmed: the malformed-payload contract behaves exactly as your PR body describes, the reserve policy is unchanged from main on both paths (so it is not a new amplification surface), and zigzag/bool conversions mirror the scalar decoders.

Final numbers on the new columnar bench (#318, 1024-element columns):

	#314 alone	with this
column_batch/decode	−24.66%	−36.23%
column_batch/decode_view	−15.57%	−20.48%
packed_signed/decode (your target)	−0.06%	−7.62%
mesh (controls)	−2.13% / −3.00%	+1.73% / −3.61%

packed_signed reading −0.06% under #314 and −7.62% under yours is about as clean a separation as this harness produces: each patch moves only the regime it targets, and nothing else.

I left the one advisory alone — check_bulk_u32 asserts value parity on Ok but not the partial vector on error. Your claim holds by construction and the partial is discarded on failure, so it's a documentation-vs-test gap, not a defect. Worth a follow-up if you're inclined, not a blocker.

Thanks for both of these — careful work, accurate PR bodies, and you were right that our benchmarks were underselling them.