perf: keep droppy error/default construction off per-message happy paths - #952
Conversation
Same family as #949, now on the paths that run per inbound message. SignalProtocolError carries drop glue (String and Box<dyn Error> variants), so every ok_or(SignalProtocolError::...) on a present-field path built the enum and paid its drop: - SignalMessage/PreKeySignalMessage/SenderKeyMessage try_from and decode_ciphertext (every 1:1 and group message parse) - group_cipher sender_chain_key lookups (group encrypt/decrypt/skdm); the format! siblings there already used ok_or_else - sender_keys seed_to_array (per cached key on record load) Also read_attributes built a default ValueRef eagerly per attribute via unwrap_or, and collect_unique_index_macs sorted owned Vec<u8> MACs after a realloc-grown collect: sort borrowed slices first, then materialize only the keepers into an exact-sized Vec. let-else/match instead of ok_or_else because clippy's unnecessary_lazy_evaluations would revert the lazy form for unit variants, ignoring drop-glue cost (see #949). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T53MPuGRg4kvRjRxW6fUvd
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR refactors error-handling patterns from ChangesOption/Result Pattern Refactor and Dedup Allocation
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested labels: Look, this diff is basically stripping out combinator soup and replacing it with 🚥 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 | One-liner change: unwrap_or(ValueRef::String) to match, avoiding eagerly constructing the CompactString-backed default on every attribute in the hot attribute-parsing loop. Semantically identical. |
| wacore/libsignal/src/protocol/group_cipher.rs | Three ok_or(SignalProtocolError::InvalidSenderKeySession) to let-else conversions on the encrypt, decrypt, and SKDM-build paths. Semantically identical; avoids drop-glue construction on the happy path. |
| wacore/libsignal/src/protocol/protocol.rs | Six ok_or(SignalProtocolError::InvalidProtobufEncoding) to let-else conversions across SignalMessage, PreKeySignalMessage, and SenderKeyMessage parsing. All changes are mechanically equivalent. |
| wacore/libsignal/src/protocol/sender_keys.rs | Single ok_or to let-else in seed_to_array, which runs on every cached message key during group decrypt record load. Semantically identical. |
| wacore/src/appstate_sync.rs | Large-patch path of collect_unique_index_macs now sorts/deduplicates borrowed &[u8] fat pointers before materializing unique MACs into an exactly-sized Vec, eliminating per-duplicate to_vec allocations. Public signature and test coverage unchanged. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Inbound message] --> B{Message type?}
B -->|1:1| C[SignalMessage::try_from]
B -->|PreKey| D[PreKeySignalMessage::try_from]
B -->|Group| E[SenderKeyMessage::try_from]
C --> C1[let Some ratchet_key else Err]
C1 --> C2[let Some counter else Err]
C2 --> C3[let Some ciphertext else Err]
D --> D1[let Some base_key else Err]
D1 --> D2[let Some identity_key else Err]
D2 --> D3[let Some message else Err]
E --> E1[let Some chain_id else Err]
E1 --> E2[let Some iteration else Err]
E2 --> E3[let Some ciphertext else Err]
F[Group encrypt/decrypt] --> G[group_cipher.rs]
G --> G1[let Some sender_chain_key else Err]
G1 --> G2[step_with_message_key]
H[collect_unique_index_macs large patch] --> H1[Vec of borrowed fat pointers]
H1 --> H2[sort_unstable by byte content]
H2 --> H3[dedup - no alloc dropped]
H3 --> H4[collect into exact-sized Vec]
I[read_attributes hot loop] --> I1[match read_value]
I1 -->|Some| I2[use value directly]
I1 -->|None| I3[construct default ValueRef]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Inbound message] --> B{Message type?}
B -->|1:1| C[SignalMessage::try_from]
B -->|PreKey| D[PreKeySignalMessage::try_from]
B -->|Group| E[SenderKeyMessage::try_from]
C --> C1[let Some ratchet_key else Err]
C1 --> C2[let Some counter else Err]
C2 --> C3[let Some ciphertext else Err]
D --> D1[let Some base_key else Err]
D1 --> D2[let Some identity_key else Err]
D2 --> D3[let Some message else Err]
E --> E1[let Some chain_id else Err]
E1 --> E2[let Some iteration else Err]
E2 --> E3[let Some ciphertext else Err]
F[Group encrypt/decrypt] --> G[group_cipher.rs]
G --> G1[let Some sender_chain_key else Err]
G1 --> G2[step_with_message_key]
H[collect_unique_index_macs large patch] --> H1[Vec of borrowed fat pointers]
H1 --> H2[sort_unstable by byte content]
H2 --> H3[dedup - no alloc dropped]
H3 --> H4[collect into exact-sized Vec]
I[read_attributes hot loop] --> I1[match read_value]
I1 -->|Some| I2[use value directly]
I1 -->|None| I3[construct default ValueRef]
Reviews (1): Last reviewed commit: "perf: keep droppy error/default construc..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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 5 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Performance optimization replacing eager error/default construction with lazy alternatives in hot paths. No behavior change.
Re-trigger cubic
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | send_and_receive[1] |
1,169.1 µs | 823.9 µs | +41.9% |
| 👁 | Memory | bench_collect_unique_index_macs[1000] |
55.3 KB | 70.3 KB | -21.42% |
| 👁 | Simulation | bench_unpack_uncompressed |
183.1 ns | 241.4 ns | -24.17% |
| 👁 | Simulation | reconnect |
2.1 ms | 2.3 ms | -10.45% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/whatsapp-rust-allocator-api-prw9c4 (cbea780) with main (0ce4907)
📦 Binary size report
.text per crate
Baseline: |
2 participants
Summary
Follow-up to #949, applying the same eager-construction fix to the three remaining hot-path families found by mining the CodSpeed flamegraphs of the heaviest benchmarks plus a code sweep:
1. libsignal message parsing (runs per inbound message).
SignalProtocolErrorcarries drop glue (InvalidArgument(String),BackendError(_, Box<dyn Error>)…), so each.ok_or(SignalProtocolError::...)on a present-field path constructed the enum and paid its drop:SignalMessage::try_from/decode_ciphertext— every 1:1 messagePreKeySignalMessage::try_from— every pkmsgSenderKeyMessage::try_from/decode_ciphertext— every group messagegroup_cipher.rssender_chain_key()lookups — group encrypt/decrypt/SKDM build. Theformat!siblings on adjacent lines already useok_or_else, confirming these unit-variant sites were simply missed.sender_keys.rsseed_to_array— per cached message key on record load (every group decrypt)2. Decoder attribute loop (hottest loop in the receive path).
read_attributesbuilt the defaultValueRef::String(NodeStr::Borrowed(""))eagerly per attribute viaunwrap_or—ValueRefhas drop glue viaCompactString. This site was missed by #949.3.
collect_unique_index_macs(appstate patch processing). The large-patch path collected ownedVec<u8>MACs through a realloc-grownVec(flamegraph: ~12% in the realloc chain), then sorted the 24-byte headers. Now sorts/dedups borrowed slices and materializes only the keepers into an exact-sizedVec— no realloc growth, no alloc+drop for duplicates, 16-byte scratch elements. Public signature unchanged.As in #949,
let-else/matchis used instead ofok_or_else/unwrap_or_elsebecause clippy'sunnecessary_lazy_evaluationsreverts the lazy form for unit variants, ignoring drop-glue cost.No behavior change — same error variants on the same failure conditions, same MAC set returned.
Validation
cargo fmt/cargo clippy --all-targetson affected crates: clean (only the pre-existingchrono::Local::nowwarning, untouched by this diff)cargo test -p wacore-libsignal -p wacore-binary -p wacore: all pass (239+ tests)bench_unmarshal_large)bench_dm_decrypt_subsequent_message,bench_group_decrypt/encrypt_message,bench_attr_parser,bench_unmarshal_*,bench_collect_unique_index_macs[1000]🤖 Generated with Claude Code
https://claude.ai/code/session_01T53MPuGRg4kvRjRxW6fUvd
Generated by Claude Code