Conversation
…AC dedup Two gaps on the inbound side, benched before any optimization: - decode_plaintext (unpad + prost decode): runs on every received message and is the inbound mirror of the already-benched encode_and_pad. Shapes match the send-side bench plus an inline-SKDM group first-message. - collect_unique_index_macs: the O(N²) linear-scan dedup feeding the batched previous-value-MAC lookup, extracted from its two inline copies (inbound process_patch_list and outbound build_patch) into a pure function so it can be measured. At the 1000-mutation resume-sync upper bound the scan takes ~1.5ms; a HashSet swap measured 6-120% slower at small N in this codebase, so both ends are pinned (N=10 and N=1000) before deciding. No behavior change: the extraction is byte-identical logic. https://claude.ai/code/session_01EoJjbyorpCARCBTZSmUMRr
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR extracts a common MAC deduplication pattern into a reusable ChangesPerformance instrumentation and refactoring
🎯 2 (Simple) | ⏱️ ~12 minutes Possibly Related PRs
Suggested Labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
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/examples/profile_decode.rs">
<violation number="1" location="wacore/examples/profile_decode.rs:61">
P3: CLI parsing uses `.unwrap()` outside tests, causing avoidable panic on invalid `iters` input.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| fn main() { | ||
| let mut args = std::env::args().skip(1); | ||
| let shape_name = args.next().expect("usage: profile_decode <shape> [iters]"); | ||
| let iters: usize = args.next().map(|s| s.parse().unwrap()).unwrap_or(20_000); |
There was a problem hiding this comment.
P3: CLI parsing uses .unwrap() outside tests, causing avoidable panic on invalid iters input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At wacore/examples/profile_decode.rs, line 61:
<comment>CLI parsing uses `.unwrap()` outside tests, causing avoidable panic on invalid `iters` input.</comment>
<file context>
@@ -0,0 +1,67 @@
+fn main() {
+ let mut args = std::env::args().skip(1);
+ let shape_name = args.next().expect("usage: profile_decode <shape> [iters]");
+ let iters: usize = args.next().map(|s| s.parse().unwrap()).unwrap_or(20_000);
+
+ let padded = MessageUtils::pad_message_v2(shape(&shape_name).encode_to_vec());
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/benches/appstate_sync_benchmark.rs`:
- Around line 42-47: Add a second benchmark variant that simulates realistic
duplicate index MACs (e.g., 50% duplicates) so you can measure early-exit
behavior; modify or add to bench_collect_unique_index_macs to call
setup_mutations with a generator that produces half-duplicate MACs and pass
those mutations into collect_unique_index_macs (keep the existing worst-case
args [10, 1000] and add a new inputs set or bench with the duplicate dataset so
you can compare timings).
In `@wacore/examples/profile_decode.rs`:
- Around line 11-56: The shape function duplicates benchmark message shapes;
extract the shared variants into a single reusable function (e.g.,
message_shape) in a new module (benches/common.rs) and have both shape (in
profile_decode.rs) and the benchmark file (message_utils_benchmark.rs) call that
shared message_shape; move the "text_reply", "media_refs", "large_text", and
"group_skdm_text" match arms into message_shape and replace the local match in
shape with a call to message_shape(name) to eliminate duplication and keep
shapes synchronized.
In `@wacore/src/appstate_sync.rs`:
- Around line 26-42: The function collect_unique_index_macs is declared pub but
is only used within the wacore crate; change its visibility to pub(crate) by
updating its signature to pub(crate) fn collect_unique_index_macs(...) to
restrict the API surface; locate the function by its name in
wacore/src/appstate_sync.rs (the function with the let-chain over m.record /
rec.index / ind.blob and linear-scan dedup) and run a quick grep/cargo build to
ensure no external crate references break.
🪄 Autofix (Beta)
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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 482f1997-8276-4d4f-8740-2a910fae4bc2
📒 Files selected for processing (5)
wacore/Cargo.tomlwacore/benches/appstate_sync_benchmark.rswacore/benches/message_utils_benchmark.rswacore/examples/profile_decode.rswacore/src/appstate_sync.rs
Comment on lines
+42
to
+47
| #[divan::bench(args = [10, 1000])] | ||
| fn bench_collect_unique_index_macs(bencher: divan::Bencher, n: usize) { | ||
| bencher | ||
| .with_inputs(|| setup_mutations(n)) | ||
| .bench_refs(|mutations| black_box(collect_unique_index_macs(black_box(mutations)))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Benchmark setup looks correct.
The args [10, 1000] cover both ends—typical incremental patches and the resume-sync upper bound. Your math checks out: 1000 distinct items hits ~500k Vec<u8> comparisons in the quadratic scan.
One thing: you're only testing the worst-case scenario (all distinct indices). Real patches might have some duplicate index MACs. Testing a case with, say, 50% duplicates could show how the early-exit .any() behaves under realistic load. Not critical for proving the HashSet claim, but it would give you a more complete picture.
That said, the setup does what it needs to do. We need this data before making any swap decisions.
🤖 Prompt for 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.
In `@wacore/benches/appstate_sync_benchmark.rs` around lines 42 - 47, Add a second
benchmark variant that simulates realistic duplicate index MACs (e.g., 50%
duplicates) so you can measure early-exit behavior; modify or add to
bench_collect_unique_index_macs to call setup_mutations with a generator that
produces half-duplicate MACs and pass those mutations into
collect_unique_index_macs (keep the existing worst-case args [10, 1000] and add
a new inputs set or bench with the duplicate dataset so you can compare
timings).
Comment on lines
+11
to
+56
| fn shape(name: &str) -> wa::Message { | ||
| match name { | ||
| "text_reply" => wa::Message { | ||
| extended_text_message: Some(Box::new(wa::message::ExtendedTextMessage { | ||
| text: Some("Benchmark message with a realistic amount of text content.".into()), | ||
| context_info: Some(Box::new(wa::ContextInfo { | ||
| stanza_id: Some("3EB0F4E1D2C3B4A59687".into()), | ||
| participant: Some("5511999990000@s.whatsapp.net".into()), | ||
| ..Default::default() | ||
| })), | ||
| ..Default::default() | ||
| })), | ||
| ..Default::default() | ||
| }, | ||
| "media_refs" => wa::Message { | ||
| image_message: Some(Box::new(wa::message::ImageMessage { | ||
| url: Some("https://mmg.whatsapp.net/v/t62.7118-24/abc123".into()), | ||
| direct_path: Some("/v/t62.7118-24/abc123".into()), | ||
| mimetype: Some("image/jpeg".into()), | ||
| caption: Some("Benchmark media caption".into()), | ||
| media_key: Some(vec![0xA5; 32]), | ||
| file_sha256: Some(vec![0x11; 32]), | ||
| file_enc_sha256: Some(vec![0x22; 32]), | ||
| file_length: Some(184_320), | ||
| height: Some(1280), | ||
| width: Some(960), | ||
| jpeg_thumbnail: Some(vec![0x7F; 6 * 1024]), | ||
| ..Default::default() | ||
| })), | ||
| ..Default::default() | ||
| }, | ||
| "large_text" => wa::Message { | ||
| conversation: Some("Lorem ipsum dolor sit amet 0123456789 ".repeat(108)), | ||
| ..Default::default() | ||
| }, | ||
| "group_skdm_text" => wa::Message { | ||
| sender_key_distribution_message: Some(wa::message::SenderKeyDistributionMessage { | ||
| group_id: Some("120363000000000001@g.us".into()), | ||
| axolotl_sender_key_distribution_message: Some(vec![0x33; 350]), | ||
| }), | ||
| conversation: Some("Benchmark group message with realistic text.".into()), | ||
| ..Default::default() | ||
| }, | ||
| other => panic!("unknown shape {other}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Look, we need to eliminate this code duplication.
These message shapes are byte-for-byte identical to the ones in message_utils_benchmark.rs (comparing lines 13-24 here to lines 37-50 there for text_reply, lines 25-40 here to lines 81-97 there for media_refs, lines 42-44 here to lines 98-101 there for large_text, and lines 46-53 here to lines 110-117 there for group_skdm_text). Your PR objectives explicitly say the harness must mirror the benchmark shapes to align profiles with CodSpeed baselines, which means these have to stay synchronized.
Extract these shapes to a shared module like benches/common.rs that both files can import. Right now, if someone updates a shape definition in the benchmark, they have to remember to update it here too - that's a maintenance trap we don't need.
♻️ Refactor approach
Create wacore/benches/common.rs:
//! Shared message shapes for benchmarks and profiling harnesses.
use waproto::whatsapp as wa;
pub fn message_shape(name: &str) -> wa::Message {
match name {
"text_reply" => wa::Message {
extended_text_message: Some(Box::new(wa::message::ExtendedTextMessage {
text: Some("Benchmark message with a realistic amount of text content.".into()),
context_info: Some(Box::new(wa::ContextInfo {
stanza_id: Some("3EB0F4E1D2C3B4A59687".into()),
participant: Some("5511999990000@s.whatsapp.net".into()),
..Default::default()
})),
..Default::default()
})),
..Default::default()
},
"media_refs" => { /* ... move the definition here ... */ },
"large_text" => { /* ... */ },
"group_skdm_text" => { /* ... */ },
other => panic!("unknown shape {other}"),
}
}Then update both files:
// In message_utils_benchmark.rs
+ mod common;
- fn dm_shape(shape: &str) -> wa::Message { /* ... */ }
fn recv_shape(shape: &str) -> wa::Message {
match shape {
- "group_skdm_text" => wa::Message { /* ... */ },
- other => dm_shape(other),
+ other => common::message_shape(other),
}
} // In profile_decode.rs
- fn shape(name: &str) -> wa::Message { /* ... entire match ... */ }
+
fn main() {
// ...
- let padded = MessageUtils::pad_message_v2(shape(&shape_name).encode_to_vec());
+ let padded = MessageUtils::pad_message_v2(
+ wacore::benches::common::message_shape(&shape_name).encode_to_vec()
+ );
}🤖 Prompt for 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.
In `@wacore/examples/profile_decode.rs` around lines 11 - 56, The shape function
duplicates benchmark message shapes; extract the shared variants into a single
reusable function (e.g., message_shape) in a new module (benches/common.rs) and
have both shape (in profile_decode.rs) and the benchmark file
(message_utils_benchmark.rs) call that shared message_shape; move the
"text_reply", "media_refs", "large_text", and "group_skdm_text" match arms into
message_shape and replace the local match in shape with a call to
message_shape(name) to eliminate duplication and keep shapes synchronized.
Comment on lines
+26
to
+42
| /// Unique index MACs of a patch's mutations, in first-seen order, feeding the | ||
| /// batched previous-value-MAC backend lookup. Linear-scan dedup is deliberate: | ||
| /// HashSet measured 6-120% slower at small N in this codebase, so don't switch | ||
| /// without benchmarking both ends (patches carry up to ~1000 mutations). | ||
| pub fn collect_unique_index_macs(mutations: &[wa::SyncdMutation]) -> Vec<Vec<u8>> { | ||
| let mut out: Vec<Vec<u8>> = Vec::with_capacity(mutations.len()); | ||
| for m in mutations { | ||
| if let Some(rec) = &m.record | ||
| && let Some(ind) = &rec.index | ||
| && let Some(index_mac) = &ind.blob | ||
| && !out.iter().any(|v| v == index_mac) | ||
| { | ||
| out.push(index_mac.clone()); | ||
| } | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if collect_unique_index_macs is referenced outside wacore.
# Search for external references (exclude wacore/ directory)
rg -n 'collect_unique_index_macs' --type rust -g '!wacore/**'Repository: oxidezap/whatsapp-rust
Length of output: 48
File: wacore/src/appstate_sync.rs (lines 26-42) — tighten pub visibility for collect_unique_index_macs
collect_unique_index_macs is pub, but there are no references to it outside the wacore crate, so switch to pub(crate) unless there’s an intentional cross-crate API contract. Implementation + let-chains and the linear-scan rationale are fine—keep that.
🤖 Prompt for 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.
In `@wacore/src/appstate_sync.rs` around lines 26 - 42, The function
collect_unique_index_macs is declared pub but is only used within the wacore
crate; change its visibility to pub(crate) by updating its signature to
pub(crate) fn collect_unique_index_macs(...) to restrict the API surface; locate
the function by its name in wacore/src/appstate_sync.rs (the function with the
let-chain over m.record / rec.index / ind.blob and linear-scan dedup) and run a
quick grep/cargo build to ensure no external crate references break.
Source: Coding guidelines
jlucaso1
force-pushed
the
claude/receive-path-benchmarks-l786xn
branch
from
June 12, 2026 12:12
1042f24 to
a179b51
Compare
jlucaso1
pushed a commit
that referenced
this pull request
Jun 14, 2026
Reverts the build.rs field-boxing half of this PR. Boxing the 29 inline Message variants shrank the struct ~75%, but it monomorphized prost's encode/merge per Box<T> field and grew the binary by ~566 KiB (+5%) — too much for the struct-size win on a binary-size-gated project. Kept: decoding Message in place on the heap (`message_decode -> Box<Message>` via merge) and threading the Box through decode_plaintext -> unwrap_device_sent -> dispatch_parsed_message. That is where the ~2x decode speedup comes from (it removes the full-struct memcpys #857 measured), and it carries negligible binary cost. The construction sites revert to plain values. https://claude.ai/code/session_01XHsbPwjaCRDHDL69HbEgR8
jlucaso1
pushed a commit
that referenced
this pull request
Jun 14, 2026
`wa::Message` was ~3.8 KiB: the sum of ~110 optional content fields, of which exactly one is ever set per message. Profiling the inbound decode (#857) attributed ~60% of it to moving and dropping that struct. Box the remaining inline message-typed content fields (prost already boxes the ones in recursion cycles). `Message` drops from 3784 to 952 bytes (-75%), so: - every `Arc<Message>` delivered to library users is ~4x cheaper to allocate and hold (a direct win for high-throughput consumers), - decode moves/drops a far smaller struct, and - the retry-cache / reporting-token clones copy less. This carries ~no binary cost (waproto+prost .text 1,729,838 B vs main 1,736,322 B; merge_field instantiation count unchanged at 16): boxing a field that is only used boxed makes prost emit the Box<T> decode/encode tree instead of the inline one, not in addition to it. BREAKING CHANGE: the boxed content fields are now `Option<Box<T>>`; construction sites wrap the value in `Box::new(..)` (reads auto-deref unchanged). The two SenderKeyDistribution fields stay inline (small, group-send hot path). https://claude.ai/code/session_01XHsbPwjaCRDHDL69HbEgR8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Standalone callgrind harness for the inbound decode path, used to generate the receive-side flamegraphs after #856 landed.
Why not profile the bench binary directly
Running
message_utils_benchmarkunder callgrind buries the signal: divan's timer-overhead calibration accounts for 83% of collected instructions, and the actual bench iterations are noise next to it. This harness loopsdecode_plaintexton a single shape (20k iterations by default) so the profile is dominated by the path under study:Shapes mirror
bench_decode_plaintext(text_reply,media_refs,large_text,group_skdm_text), so profiles line up with the CodSpeed baseline points.What the first profile surfaced
~60% of inbound decode cost is moving and dropping the 3,784-byte
wa::Messagestruct, not protobuf parsing: three full-struct memcpys per decode (return frommessage_decode, return fromdecode_plaintext, move at the caller — 7.78M Ir each at 20k iters), plus ~24% allocator traffic and ~10% indrop_in_place. That points at a follow-up: decode intoBox<wa::Message>so the moves become pointer-sized — to be proposed separately and measured by CodSpeed against the #856 baseline.Not a benchmark and not compiled into the library —
examples/only builds on demand.https://claude.ai/code/session_01EoJjbyorpCARCBTZSmUMRr
Generated by Claude Code