perf(binary): unpack packed values through a byte-pair table - #1214
Conversation
The two unmarshal benches decode nodes with neither a JID nor a hex id, so read_jid_pair and read_packed, which dominate a small-stanza profile, were measured by nothing. bench_unmarshal_ack fixes that: a 30-byte ack whose bytes carry JID_PAIR, NIBBLE_8 and HEX_8, which costs 113 ns against 90 ns for the existing small node. That 22 ns of decode path had no coverage at all. bench_unmarshal_fanout adds the repeated shape, eight devices at 1.35 us. Measured while evaluating a visitor API that would decode without building the tree, and the benches are what survived. The tree turns out to cost almost nothing: with a heap profiler, the ack allocates exactly once, 168 bytes for its attribute array, because the decoder already borrows frame strings and keeps ValueRef::Jid lazy rather than rebuilding the JID. An isolated malloc plus free of that size is 4.37 ns, 3.9% of the decode, which independently reproduces on x86-64 what a wasm profile had put at under 4%. There is no tree cost to remove.
📝 WalkthroughSummary by CodeRabbit
WalkthroughPacked hex and nibble decoding now uses lookup tables. Tests cover round trips across SIMD boundaries and invalid values. Binary benchmarks add ACK and device-fanout payloads. ChangesPacked decoding optimization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| wacore/binary/src/decoder.rs | Replaces scalar hex and nibble expansion with pair-table copies while retaining invalid-nibble validation. |
| wacore/binary/tests/packed_equivalence.rs | Adds packed-value round-trip coverage across chunk boundaries and checks rejection of invalid nibble values. |
| wacore/binary/benches/binary_benchmark.rs | Adds representative ack and device-fanout decode benchmarks. |
Reviews (4): Last reviewed commit: "test(binary): reject an invalid nibble f..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 487ed867d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Profiling the new ack benchmark put read_packed at 30% of a small-stanza decode, far above where a wasm profile had it. The reason is that its SIMD path only handles 16-byte chunks, and real packed values never reach that: a 13-digit phone number packs to 7 bytes, a 20-character id to 10. Every packed value on a small stanza takes the scalar loop, which does two shifts, two lookups and two bounds-checked stores per input byte. A 256-entry table mapping each byte to its two output characters makes that one load and one 2-byte store. Nibble values 12, 13 and 14 encode nothing, so they are marked in the table and fall back to the scalar walk, which reports which half was bad, unchanged. On the ack: 15.5% fewer instructions (9.375G to 7.925G over 5M decodes, measured with perf) and 14.5% less wall time (114.9 ns to 98.3 ns). The fanout drops 6.7%. The two benches without packed values do not move, which is where the gain should come from and does. The equivalence tests pass against both the old and the new decoder, so they pin behavior rather than implementation.
jlucaso1
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Confidence score: 4/5
- In
wacore/binary/benches/binary_benchmark.rs, the fanout benchmark comment and implementation diverge (read_jid_pairruns only for the parentto, while child device JIDs useread_ad_jid), which can mislead performance conclusions about parser hot paths—align the benchmark logic or update the comment so measured behavior matches the stated intent. - In
wacore/binary/benches/binary_benchmark.rs(create_small_nodedocs), the comment currently understates exercised paths even though the fixture appears to hitread_jid_pairandread_packed; this can confuse future benchmark interpretation—reword the doc comment to reflect what the fixture actually covers.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="wacore/binary/benches/binary_benchmark.rs">
<violation number="1" location="wacore/binary/benches/binary_benchmark.rs:28">
P3: This doc comment claims create_small_node exercises neither read_jid_pair nor read_packed, but the existing fixture's JID (user@s.whatsapp.net) and numeric id (12345) already exercise these paths. Consider rewording to clarify this bench measures a more representative combination/frequency of already-covered operations rather than previously-uncovered paths, so CodSpeed results aren't misinterpreted.</violation>
<violation number="2" location="wacore/binary/benches/binary_benchmark.rs:37">
P2: The fanout benchmark does not repeat `read_jid_pair` per child as the comment claims: device JIDs take the `AD_JID`/`read_ad_jid` path, while only the single parent `to` value takes `read_jid_pair`. This makes the benchmark unsuitable for the stated repeated JID_PAIR coverage unless the fixture is changed to use bare JIDs, or the comment is corrected to describe the AD_JID path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Two claims in the fixture comments were wrong, both checked against the encoded bytes rather than assumed this time. create_small_node does reach read_jid_pair and read_packed: it encodes JID_PAIR for user@s.whatsapp.net and NIBBLE_8 for the id 12345. What no bench reached was the hex half of read_packed, because the large fixture's lowercase abcdef classifies as raw bytes, not HEX_8. So the ack adds hex coverage and a representative combination, not two uncovered paths. The fanout does not repeat read_jid_pair either. Device-qualified JIDs encode as AD_JID, so what repeats per child is the packed decode and the AD path. Neither correction touches the measurement: read_packed is still 30% of the ack decode and the table still removes 15.5% of its instructions.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 5/5
- In
wacore/binary/tests/packed_equivalence.rs, the invalid-nibble coverage currently uses only a single packed byte, so it misses the 16-byte chunk scalar fallback path called out for nibbles 12/13/14; this leaves a small regression risk that those lanes could behave differently without being caught by CI — add a multi-byte (>16-byte) invalid-nibble case to exercise that fallback path directly.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="wacore/binary/tests/packed_equivalence.rs">
<violation number="1" location="wacore/binary/tests/packed_equivalence.rs:37">
P3: The invalid-nibble test only ever feeds a single packed byte, so it exercises the remainder scalar path, never the 16-byte-chunk scalar fallback that the PR calls out for nibble 12/13/14. Consider adding a case with packed length >= 16 containing an invalid low nibble so the chunk double-pass validation is actually covered.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The invalid-nibble case fed a single packed byte, which only ever reaches the scalar remainder. Under the simd feature, a bad nibble in the first 16 bytes takes a different route: the chunk fails its vector validation and falls back to a scalar double pass. That route had no test. A 20-byte frame with the bad nibble at byte 5 covers it. Both lengths now pass with and without the feature, and against the pre-table decoder, so the assertion is about the format rather than about either implementation.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/binary/tests/packed_equivalence.rs`:
- Around line 36-59: Update an_invalid_nibble_is_still_rejected to pass both
short and long invalid frames through the alternate decoder entry point in
addition to unmarshal_ref. Assert that the alternate decoder rejects each frame
and includes the offending nibble in its error, preserving the existing checks
for unmarshal_ref.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3cce3522-2f85-4fd9-9e61-743863b61fd8
📒 Files selected for processing (1)
wacore/binary/tests/packed_equivalence.rs
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Behavior-preserving performance refactor: packed hex/nibble decoding replaced by lookup tables, with new equivalence tests pinning boundary and invalid-nibble behavior. No public API, policy, or operational tradeoff changes; only internal decoder plus benchmarks/tests.
Re-trigger cubic
Merging this PR will degrade performance by 8.06%
Performance Changes
Comparing 
Footnotes
|
|
@codspeedbot explain this regression |
|
| instructions | memory access | cache miss | total | |
|---|---|---|---|---|
BASE (81c26b3) |
1.165 µs | 1.667 µs | 0.083 µs | 2.915 µs |
HEAD (f9cd9a1) |
1.187 µs | 1.889 µs | 0.094 µs | 3.171 µs |
The instruction count itself moved (+1.9%), so this is not a cache-model artifact of a runner swap — and CodSpeed's environment-diff section confirms this benchmark was compared on matching runners, unlike the wacore/binary, wacore/libsignal and wacore/appstate benches.
The same value already oscillates on main
That bench file, SignalStoreCache and ProtocolAddress have had no commits across the window below (git log 786c635..81c26b3 on those paths is empty), yet:
| main commit | identity_probe_misses[8] |
|---|---|
| #1209 786c635 | 3.003 µs |
| 05c98a1 | 3.003 µs |
| 9a78719 | 3.073 µs |
| #1210 37ae410 | 2.915 µs |
| #1211 0f78723 | 3.141 µs |
| #1212 2b60761 | 2.915 µs |
| #1213 81c26b3 (this PR's base) | 2.915 µs |
| this PR | 3.171 µs |
A 7.8% spread on identical code. The head value sits at the top of a range main already visits.
Reproduced locally
I built and ran the bench under codspeed run --mode simulation on this branch and on 81c26b3, same machine, and read the raw callgrind counters for identity_probe_misses[8]:
- base binary:
Ir=10,643 Dr+Dw=3,649 D1mr=49 - head binary:
Ir=10,587 Dr+Dw=3,619 D1mr=45
Head is marginally cheaper on every counter. Then, running the unchanged head binary five times, it alternates between Ir=10,587 and Ir=10,643 — the two values I had attributed to head and base. The binary is fixed; the number is not.
Why it wobbles
SignalStoreCache.identities is a std::collections::HashMap<Arc<str>, _> with the default RandomState, so its SipHash keys are seeded per process. Bucket layout, probe length and the resulting data accesses differ every run. The bench's own DetHashMap precaution covers only the address_keyed_map_probe control map, not the cache under test. With 8 keys the per-iteration work is small enough that one extra probe or a differently placed group moves the total several percent; identity_probe_misses[64] amortizes it away and came in at +0.5%, and identity_probe_hits[8] was byte-identical between the two runs. Only the smallest, most seed-sensitive workload moved, which is the signature of noise rather than a change.
For completeness, this PR touches read_packed in wacore/binary/src/decoder.rs; signal_address_probe_benchmark never calls the decoder.
Suggestion
Acknowledge this one on CodSpeed. If you want the bench to stop generating these, the fix is to remove the seed dependence rather than the workload — letting SignalStoreCache be constructed with a fixed BuildHasher in benches would make [8] as reproducible as the rest of the suite. I did not make that change here since it means touching production store code, which is outside this PR.
Summary
- Warming up the performance wizard...
- Investigating the reported regression — Reproduced identity_probe_misses[8] locally on base and head: the benchmark is nondeterministic, the PR is not the cause
- Handling comment — Comment handling finished successfully
Learnings updated. See what CodSpeed remembers about this repository.
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.
NodeRef::content was Option<Box<NodeContentRef>>, so every node carrying any content allocated, including the common leaf whose content is a borrowed slice of the frame and allocates nothing itself. The box existed only to keep the field pointer-sized. A device fanout pays one per node: a native profile puts malloc, free and NodeRef's drop glue at 11.3% of that decode, against 3.9% on an ack, which is why #1214 measuring only the ack concluded the tree was nearly free. Dropping the box does not create an infinite type, because the recursion goes through Box<[NodeRef]>. NodeRef grows from 48 to 72 bytes, and the memcpy that buys back does not show up: every benchmark improves, with the gain tracking how many nodes carry content. Allocations per decode: fanout 45 to 27, large 50 to 26, ack unchanged at 1 since it has no content. Time over 2M iterations, pinned: fanout -10.8%, large -11.0%, ack -6.4%, small -5.4%. Instructions on the fanout drop 9.5% under callgrind. The field is public, so this reaches 22 files outside wacore/binary. All of it is as_deref() becoming as_ref(); no logic moves.
1 participant
Summary
Decoderis entirelypub(crate), so the only way to use this crate's parser isunmarshal_ref, which hands back the whole tree. A consumer that projects the node straight into another representation, as the WASM bridge serving Baileys does, builds that tree only to take it apart. The proposal behind this work was a visitor API letting such a caller read a decoded node without materializing anything, on the same grounds as #1211 and #1213.The measurement refused the visitor and found something better. The tree turns out to cost almost nothing, so removing it was never worth much. But the benchmark written to evaluate it exposed a path nobody was measuring, and profiling that path found a real 15% of a small-stanza decode sitting in plain sight. That fix helps every caller, including this repository's client, which is what a visitor never would have done.
Measurement
bench_unmarshal_ackis a 30-byte ack whose encoded bytes carryJID_PAIR,NIBBLE_8andHEX_8at once;bench_unmarshal_fanoutrepeats the packed andAD_JIDdecodes across eight devices.To be precise about what that adds, since an earlier revision of this description overstated it:
create_small_nodealready reachesread_jid_pairand the nibble half ofread_packed. What no bench reached is the hex half, because the large fixture's lowercaseabcdefclassifies as raw bytes rather thanHEX_8. So the ack adds hex coverage plus a combination and frequency that match the wire, not two uncovered paths.bench_unmarshal_ack(new, 30 B)bench_unmarshal_small(existing)bench_unmarshal_fanout(new)bench_unmarshal_large(existing)Figures are
fastest, pinned to one core. Allocation counts come from a heap profiler run separately, since it perturbs timing.Why the visitor was refused. The ack allocates exactly once, for its
Box<[(NodeStr, ValueRef)]>of three attributes. Nothing else, because the decoder already does most of what a visitor would:NodeStr::Borrowedkeeps frame strings borrowed,ValueRef::Jidholds components rather than rebuilding the JID string, and unpacked hex and nibble values fit inline in aCompactStringwithout hitting the heap. An isolated malloc plus free of 168 bytes measures 4.37 ns against the 113 ns decode, so that single allocation is 3.9%. That independently reproduces on x86-64 native what a wasm profile had already put at under 4% fordlmalloc.What a visitor could still take is that one allocation plus repositioning roughly 33 bytes of writes for the reconstructed values. The unpacking and UTF-8 validation underneath are protocol work it moves rather than removes. Against a 70 ns gap in 217, it does not close.
What this does not cover. These are native x86-64 numbers; the profile that motivated the proposal is wasm32, where absolute costs differ. It also does not measure a visitor, because none was built: constructing the API to discard it was not worth it once the allocation count came back at one. If someone wants to reopen this, the number to beat is 4%.
Two further suspicions were checked and did not hold:
read_packedalready has a SIMD path, and its 254-byte stack buffer produces nomemset(LLVM elides it, confirmed in the disassembly).What the profile found
With the ack benchmark in place,
perfputsread_packedat 30% of the decode, well above the 7.7% a wasm profile had shown. The cause is that its SIMD path only handles 16-byte chunks, and real packed values never reach one: a 13-digit phone number packs to 7 bytes, a 20-character id to 10. Every packed value on a small stanza takes the scalar loop, which does two shifts, two lookups and two bounds-checked stores per input byte.A 256-entry table mapping each byte to its two output characters turns that into one load and one 2-byte store. Nibble values 12, 13 and 14 encode nothing, so they are marked in the table and fall back to the scalar walk, which still reports which half was bad.
bench_unmarshal_ackbench_unmarshal_fanoutbench_unmarshal_small(no packed values)bench_unmarshal_large(no packed values)The two benches without packed values not moving is the control: the gain comes from the path it should. Instruction counts are from
perf stat, which varies by under 1000 across 9 billion, so the -15.5% is exact rather than estimated. Wall time gains less than instruction count does, because the decode is latency-bound rather than throughput-bound; on wasm, where the consumer runs, instructions weigh more.Why this is not #1211 or #1213
Those opened a door for a consumer with a legitimately different profile, at no cost to anyone who ignores it. This would have too, and it carries none of what sank #1212: no global state, no bound, no eviction, no policy. The criterion that did apply from #1212 applies here as well, and is worth saying plainly: there is no internal beneficiary. This repository's client wants the tree and uses all of it.
That criterion alone would not have been decisive, since #1213 has no internal beneficiary either and was still right to land. What decided it was the size of the prize.
What stays
The benches. They are tracked by CodSpeed from here on, so a regression in
read_jid_pairorread_packedbecomes visible instead of silent, for this repository's client and for any consumer. The 22 ns betweenbench_unmarshal_ackandbench_unmarshal_smallis the path that had no coverage.Validation
No existing test or bench was edited.
packed_equivalencepins the decode across the SIMD boundary and the invalid-nibble error, and passes against both the old and the new decoder, so it fixes behavior rather than implementation. Full matrix left to CI.Follows #1211, #1212 and #1213.