perf(send): memoize the group phash on the device-list memo entry - #840
Conversation
Every warm group send rebuilt the phash set (one clone per device, hosted filter, sort-dedup) and re-hashed it (sort + SHA-256), ~45% of the post-#838 warm-send window, even though the inputs are pinned by the per-group device memo from #824: the device set IS the memo entry and the sending jid is stable per group. ResolvedGroupDevices bundles the resolved set with a lazily computed phash in a OnceLock behind the entry's Arc. The memo inherits the device memo's invalidation for free: any topology change produces a new entry (cold phash), re-stamps keep the same Arc (warm phash survives write storms on unrelated groups), and no new write path or invalidation rule is introduced, leaving the #824 invariants untouched. The cell pins the sending jid, so a sending-identity change recomputes instead of serving a stale hash. A phash is 10 bytes, inline in CompactString end to end (the stanza attr accepts it directly): a warm send costs one pointer-free copy, zero allocations, instead of N jid clones + two sorts + a hash. The cold/broadcast paths are unchanged. The bench fixture now builds the resolved set in setup and pre-warms it, measuring the warm steady state like production.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis refactor adds ResolvedGroupDevices (device list + sender-pinned, lazily cached phash) and swaps memo storage/returns to Arc, updating SKDM resolution, group stanza phash paths, tests, and benchmark pre-warming to use the new wrapper. ChangesResolvedGroupDevices memoization refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/send_receive_benchmark.rs`:
- Around line 631-636: The warm-up call to ResolvedGroupDevices::phash is
currently discarding its Option return (let _ = resolved.phash(&alice.jid);) so
a failed warm will be ignored; change this to validate success by unwrapping or
expecting the Option (e.g. resolved.phash(&alice.jid).expect("phash warming
failed")) so any error in phash (which returns Option<CompactString>) surfaces
during benchmark setup and ensures the steady-state path is actually warmed.
In `@wacore/src/send/resolved_devices.rs`:
- Around line 50-53: The comment about a benign race is misleading: change it to
clarify that concurrent callers may compute different phashes if their
own_sending_jid differs, and that the implementation still returns the correct
phash for each caller while the first writer wins the shared memo slot; update
the comment near Self::compute, own_sending_jid, and self.phash.set to state
"benign race: concurrent callers may compute different values when
own_sending_jid differs; first writer wins the memo slot but each caller returns
its correct computed phash."
🪄 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: d631c7a0-1a9b-46a0-8fe1-3f1cffa377f9
📒 Files selected for processing (6)
src/client/device_registry.rssrc/send.rswacore/benches/send_receive_benchmark.rswacore/src/send.rswacore/src/send/group.rswacore/src/send/resolved_devices.rs
Merging this PR will improve performance by 54.55%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | bench_group_send_256 |
53.1 KB | 18.9 KB | ×2.8 |
| ⚡ | Simulation | bench_group_send_256 |
476.8 µs | 248.7 µs | +91.76% |
| ⚡ | Memory | bench_group_send_50 |
9.4 KB | 6 KB | +55.58% |
| ⚡ | Simulation | bench_group_send_50 |
279.2 µs | 232.3 µs | +20.19% |
| 👁 | Simulation | bench_unpad_message_ref |
191.9 ns | 219.7 ns | -12.64% |
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 perf/phash-memo (03d9041) with main (e5c660f)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
On the flagged |
CodeRabbit: the fixture now expects the phash warm to succeed (a silent failure would leave the bench measuring the cold path while claiming warm), and the race comment states precisely what happens when racing firsts carry different sending jids.
|
Verified the bench_unpad_message_ref regression frame-by-frame via the CodSpeed MCP flamegraphs (main e5c660f vs PR 67e0e1f). The measured function is byte-identical in cost: unpad_message_ref self time 97.5 -> 96.7 ns (instructions 10 -> 9.2 ns, caches and memory equal). The entire +28 ns delta sits in the harness's black_box<&[u8]> frame, whose memory component went 27.8 -> 55.6 ns: exactly one extra memory-access quantum in the simulator's cost model. The new binary materializes the black_box spill with one more stack access, a thin-LTO regalloc/layout artifact of the wacore crate changing at all; neither the bench file nor messages.rs is touched by this PR. It reproduces across runs because the same crate content yields the same codegen partition, so it is deterministic per binary but non-causal to the change. Safe to acknowledge; the codegen-units=1 bench-profile follow-up eliminates this class. |
1 participant
Problem
The CodSpeed flamegraph for
bench_group_send_256(post-#838) showsbuild_group_phash_set+participant_list_hashat ~212 µs of a ~475 µs warm send: one Jid clone per device, a hosted filter, a sort-dedup, then another sort and a SHA-256 over the whole set, recomputed on every send. But both inputs are already pinned: the device set is exactly the per-group device-list memo entry from #824, and the sending jid is stable per group.Change
ResolvedGroupDevices(new, in wacore::send) bundles the resolved device set with a lazily computed phash in aOnceLockthat lives behind the memo entry'sArc:Arc, so the warm phash survives write storms on unrelated groups. No new write path, no new rule: the five perf(send): memoize the per-group device list behind a topology generation #824 invariants are untouched (the entry value changed shape; the tracker, chokepoints and stamping logic are exactly as before, and all anchor tests pass against the new type).CompactString, and the stanza attr acceptsCompactStringdirectly: a warm send costs one inline copy. The cold (force_skdm) and broadcast paths keep the direct computation.The send/receive bench fixture now builds the resolved set in setup and pre-warms the memo, so the benches measure the warm steady state the way production runs it (same lesson as #838/#839: lazy warms are invisible to fresh-fixture benches).
Expected effect
bench_group_send_256/50/10simulation should drop substantially (the phash share grows as group size does; ~212 µs of the 256-member window). Production: every warm group send skips N clones, two sorts and a SHA-256.Tests
Arc::ptr_eq.ack_miss_path_does_not_heap_allocateflaked once under parallel execution and passes in isolation and on rerun; unrelated path.)