perf(appstate): index-sort dedup for large patches, O(n²) scan stays for small - #868
Conversation
collect_unique_index_macs deduped index MACs with an O(n²) first-seen linear scan. Profiling the [1000] benchmark (the resume-sync patch ceiling) showed ~87% of its 5.7ms in byte-slice equality from that scan. Keep the cache-friendly scan for small patches, where a HashSet measured 6-120% slower, and switch to a side HashSet that borrows the source MAC slices above 64 mutations. First-seen order is preserved on both paths; the existing [10]/[1000] benchmarks pin both ends and a new test asserts the paths agree.
📝 WalkthroughWalkthrough
ChangesIndex MAC Deduplication Strategy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Merging this PR will improve performance by ×10
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | bench_collect_unique_index_macs[1000] |
5,678.6 µs | 550.8 µs | ×10 |
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-pr-review-mz0gyy (fd6c7ff) with main (23130ca)
Replaces the large-N HashSet membership set with an O(n log n) sort of position indices: only a Vec<u32> of scratch (~4KB at N=1000) versus a HashSet of 32-byte MACs (~34KB), which tripped CodSpeed's memory gate. The small-N linear scan is unchanged. The index sort groups equal MACs (position-tie-broken so each run's first occurrence leads), drops the non-leaders, and restores first-seen order — same result as the scan, verified by the path-agreement test across N below and above the threshold.
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/src/appstate_sync.rs`:
- Around line 786-795: The test scan_and_sort_paths_agree currently only tests
cases with lexicographically ordered first appearances (using the generated
build function), which doesn't properly exercise the "restore first-seen order"
step. Add a test case with a non-lexicographic fixture where first-seen order
differs from byte-sort order, such as manually constructing a sequence with
distinct values that appear in a different order than their sorted order. This
ensures that any regression in the order restoration logic will be caught
immediately instead of passing silently when first-seen order happens to match
lexicographic order.
🪄 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: d6c33a4a-1283-4f5d-8a01-f5bc4dd1b98b
📒 Files selected for processing (1)
wacore/src/appstate_sync.rs
What
collect_unique_index_macs(inwacore/src/appstate_sync.rs) deduplicates a patch's index MACs in first-seen order, feeding the batched previous-value-MAC backend lookup. It used an O(n²) linear scan (out.iter().any(|v| v == index_mac)) per element.Now:
HashSetmeasured 6–120% slower at small N here).Vec<u32>.Why
A fresh CodSpeed sweep of
mainflagged this as the one heavy benchmark dominated by avoidable algorithmic work (everything else heavy is genuine SHA/curve crypto or zlib). At N=1000 (the resume-sync patch ceiling), ~87% of the 5.7 ms was byte-slice equality from the O(n²) scan (~500K 32-byte compares).An earlier revision used a
HashSet<&[u8]>, which was ×17 faster but added ~34 KB of transient allocation per patch and tripped CodSpeed's memory gate. The index sort keeps the bulk of the win with only a ~4 KBVec<u32>.Results (CodSpeed, fd6c7ff vs main)
bench_collect_unique_index_macs[1000]: 5,678 µs → 550.8 µs (×10).Correctness
dedup_testsassert the scan and sort paths agree acrossn = 8, 64, 65, 1000(with duplicates), that mutations without an index MAC are skipped, and — exercising the sort path specifically — that first-seen order is restored when it runs opposite to byte-sort order (so an order-restoration bug can't pass by coincidence).No wire/behavior change; pure internal dedup. Ran
cargo fmt/clippyand the new tests locally; full suite + CodSpeed on CI.