perf(binary): validate packed output with the wire path's validator - #1215
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesPacked decoding
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| wacore/binary/src/decoder.rs | Reuses smoothutf8 for packed decoder output without changing reachable decoding semantics. |
Reviews (2): Last reviewed commit: "perf(binary): validate packed output wit..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Localized performance-only swap of the UTF-8 validator on internally generated ASCII bytes, retaining the check and preserving behavior; no public, data, or operational tradeoff remains.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
read_packed unpacks into a stack buffer and then runs the bytes through std::str::from_utf8 before building the CompactString. Those bytes came from the two lookup tables a few lines above, whose entries are all ASCII, so the check can never fail. It is the only from_utf8 in the file validating data the decoder produced rather than data off the wire. smoothutf8 is what read_string already uses for wire bytes, for the short strings that dominate here too. Swapping it in cuts 8.6% of the ack's instructions (1587 to 1450 per decode, perf stat over 2M). Native wall time does not move, so this lands on instruction count, which is what CI measures and what weighs most on wasm. The check is kept rather than removed. from_utf8_unchecked would take another 3% and the repo has precedent for it, but that trades a verified invariant for a documented one to buy a slice of a figure that does not show up in native time. Worth recording, since it surprised me: a nibble of 15 in the middle of a packed value decodes to a NUL that survives into the string. Neither validator rejects it, because NUL is valid ASCII, so this is unchanged behavior rather than something the swap introduces.
e2095b6 to
54c6fa5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Stacked on #1214. Review that one first; this diff is one line plus its comment.
Summary
read_packedunpacks hex and nibble into a stack buffer, then runs the result throughstd::str::from_utf8before building theCompactString. Those bytes come from the two lookup tables a few lines above, whose entries are all ASCII, so the check can never fail. It is the onlyfrom_utf8in the file validating data the decoder itself produced rather than data that came off the wire, andread_stringa hundred lines up shows the contrast: that one validates frame bytes, where validating is mandatory.This swaps the validator for the one that path already uses, rather than removing the check.
Measurement
Per decode,
perf stat -r 5over 2M iterations of a 30-byte ack, pinned to one core:std::str::from_utf8)smoothutf8::from_utf8from_utf8_unchecked(ceiling, prototype)8.6% fewer instructions, capturing 74% of what removing the check entirely would buy. Native wall time does not move: the difference is inside the noise, and even the unchecked ceiling only shows 2%. The decode is latency-bound rather than throughput-bound, so this lands on instruction count, which is what CI measures and what weighs most on the wasm target where the reporting consumer runs.
The fanout shows -4.7% instructions and no time change, as expected: its packed values are a smaller share of the work.
What this does not cover. Native only. The 8.1% figure that motivated the proposal is wasm32, and I have no wasm measurement to confirm it there.
Design
The check stays.
from_utf8_uncheckedwould take the remaining 3%, and this repo has precedent for exactly that pattern in #1187, which removed the same kind of self-revalidation inAddressBuf::as_strbehind aSAFETY:comment. The difference is what it buys: there the invariant was worth the trade, here the remaining slice does not show up in native wall time at all, anddecoder.rscurrently has zerounsafe. Keeping a real check also means a future edit to either table cannot quietly produce non-ASCII.One thing worth recording, from confirming the proposal's question about the terminating zero: a nibble of 15 in the middle of a packed value decodes to a NUL that survives into the string. I verified this by hand-building a frame, and the decoded tag comes back as
[49, 53, 0, 50]. Neither validator rejects it, because NUL is valid ASCII, so this is pre-existing behavior rather than something this change introduces. Flagging it because a validator sitting right there invites the assumption that it would be caught.Validation
packed_equivalencefrom #1214 already covers both packed kinds across the SIMD boundary, odd lengths that exercise the half-byte trim, and invalid-nibble rejection, and it passes unedited. Full matrix left to CI.Follows #1214, which touched the same
read_packedfor a different reason.