docs(perf): measure allocation on the group stanza build - #1282
Conversation
An external profile put a group send at 387 allocations and 87.7 KiB per message against 166 and 39.1 KiB for a DM, and pointed at `ensure_sessions_for_devices` as the place to start. Measured here with `divan::AllocProfiler` over the existing send benches, the group path is not expensive and does not grow with group size: | send | allocs/msg | bytes/msg | | ----------------------------------- | ---------: | --------: | | `bench_dm_send` | 157 | 27.9 KB | | `bench_group_send_10` (warm) | 22 | 3.58 KB | | `bench_group_send_50` (warm) | 22 | 3.58 KB | | `bench_group_send_256` (warm) | 22 | 3.58 KB | | `bench_group_send_skdm_256` (cold) | 6,816 | 675.1 KB | Flat from 10 to 256 members, and seven times cheaper than a DM -- which follows from the shapes once they are side by side: a DM pairwise-encrypts per recipient device, a warm group send does one sender-key encrypt for everyone. The group path is cheaper per message precisely because sender keys exist. So 387 is not a per-message cost, it is an average. A distributing send is 6,816 allocations at 256 targets (~26.6 per device: one X3DH plus one pairwise encrypt, inherent -- each device needs its own copy of the sender key under its own ratcheting session). At 128 members that is ~3.4K, so redistributing about every ten messages averages to 387. The lever a reader would take from that number -- cut allocations in the send path -- is the wrong one; the quantity that moves it is redistribution frequency, which `resolve_skdm_targets_memoized` already minimizes and which is correctness, not a performance knob. `ensure_sessions_for_devices` is named explicitly because a profile points there: it is called from inside the `distribution_list` branch only, so a warm send never enters it, and its cost in an averaged profile is entirely amortized cold-path cost. No code changes. The `AllocProfiler` used to measure this is deliberately not checked into `send_receive_benchmark.rs` -- swapping the global allocator shifts the timing of every bench in that file, which would put a one-time step through each of their CodSpeed series for a number wanted only occasionally. The doc carries the three lines needed to reproduce it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JXjxfWyfvxB6WEgiCLVF3
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe documentation adds per-message allocation measurements for direct messages, warm groups, and sender-key distribution. It explains steady-state costs, redistribution scaling, redistribution frequency, and temporary allocator instrumentation. ChangesAllocation observability
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested labels: 🚥 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 |
|---|---|
| agent_docs/observability.md | Adds a scoped allocation analysis whose revised warm-send explanation is consistent with the current send pipeline and benchmark fixture. |
Reviews (3): Last reviewed commit: "docs(perf): label the distributing row a..." | 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}
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 `@agent_docs/observability.md`:
- Around line 376-382: Update the warm group send description to call it “the
cheapest measured send path” instead of “the cheapest thing this client does,”
keeping the allocation figures and surrounding comparison unchanged.
- Around line 388-393: Correct the amortization example in the observability
documentation: one approximately 3,400-allocation redistribution plus nine
22-allocation warm sends averages about 360 allocations per message, not 387.
Either change the stated average to approximately 360 or explicitly document the
additional cost required to justify 387, while preserving the surrounding
warm/distribution context.
🪄 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: c555cef5-4bf6-4df7-92d4-2ba9668f96db
📒 Files selected for processing (1)
agent_docs/observability.md
![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: ad8196260b
ℹ️ 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".
📦 Binary size report
.text per crate
Baseline: |
Review caught four claims that the measurement does not support. The numbers stand; the conclusions drawn around them did not. The benchmark is not the client send path. `run_group_send` calls `prepare_group_stanza` and marshals the node -- the group lookup, retry caching, sender-key cache access, `resolve_skdm_targets_memoized` and persistence work in `src/send/mod.rs` are all outside the measured region. Comparing 22 against a whole-client 387 was comparing two different quantities, so the section no longer claims 387 is unreproducible. Retitled to say what is measured. "A warm send never calls `ensure_sessions_for_devices`" is false for the ordinary linked account. `src/send/mod.rs` states it directly in the `initial_targets` match: own devices are never memoized warm, so own-only SKDM needs IS the warm steady state and such a send carries a nonempty `distribution_list` on every message. The 22-allocation figure is the zero-own-target case and now says so. The distributing row is not X3DH. `setup_group_send` establishes a session for every member before forcing distribution, so `ensure_sessions_for_devices` finds them present and never reaches the prekey-fetch branch. 6,816 is the SKDM encrypt fan-out plus stanza build, ~26.6 allocations per target -- not one X3DH plus one encrypt each, and not a cost this repository has measured cold. `resolve_skdm_targets_memoized` does not govern redistribution frequency. It memoizes device-set resolution; `filter_skdm_targets` against the `SenderKeyDeviceMap` decides which devices still need the key. Calling a lookup cache the correctness mechanism pointed readers at the wrong state. Also: the amortization example averages ~360, not 387 (one ~3.4K distribution plus nine 22-allocation sends over ten messages), and "the cheapest thing this client does" is narrowed to the paths actually measured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JXjxfWyfvxB6WEgiCLVF3
jlucaso1
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbf4c37a82
ℹ️ 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".
Brings in #1281, whose `warm_group_send_encoding_scale` this section cites. The reference dangled while this branch sat on a pre-merge base.
Three more from review, all confirmed against the fixtures. The cited regression test was not in this branch's tree. `perf/group-allocs` was cut before #1281 landed, so `warm_group_send_encoding_scale` existed only on main and the reference dangled for anyone reading the PR. Merged main in. The distributing row is a first-message fan-out, not a steady rotation. `establish_session` runs `process_prekey_bundle` alone -- unlike `establish_bidirectional` it never completes the round trip -- so every session still carries its `pending_pre_key` and each SKDM encryption emits a `pkmsg`, with prekey wrapping and device-identity serialization attached. 6,816 is that shape. A later rotation or reset over acknowledged sessions emits plain `SignalMessage`s and costs less; nothing here measures it. Labelled in the table and in the prose, replacing a paragraph that implied the figure covered redistribution generally. The extrapolation was in the wrong unit. `setup_group_send(n)` creates exactly one device per member, while SKDM fan-out scales with resolved devices -- a real group resolves to more targets than members. The ~3.4K figure is now stated per 128 *targets*, and the comparison notes that the external number is quoted in members whose device count is unknown, which is one more reason the decomposition is not claimable from here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JXjxfWyfvxB6WEgiCLVF3
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
2 participants
Summary
An external profile put a group send at 387 allocations / 87.7 KiB per message at 128 members and named
send::encrypt::ensure_sessions_for_devicesas the place to start.This records what
divan::AllocProfileractually measures on this repository's send benches, and — after review — what those numbers do not establish. The first revision of this PR claimed they refuted the 387; they do not, and the doc now says so explicitly.The result that survives: a group send that distributes no sender key is flat in group size — 22 allocations and 3.58 KB at 10, 50 and 256 members.
Changes
agent_docs/observability.mdNo code changes.
Cost
divan::AllocProfileras global allocator insend_receive_benchmark.rs, 50 samples, pinned to one core. Divan tallies only the benchmarked closure.Scope, stated first because it bounds every row:
run_group_sendcallsprepare_group_stanzaand marshals the node. The client send path around it — group lookup, retry caching, sender-key cache access,resolve_skdm_targets_memoized, persistence-adapter construction, the send itself — is outside the measured region.bench_dm_sendbench_group_send_10(no distribution)bench_group_send_50(no distribution)bench_group_send_256(no distribution)bench_group_send_skdm_256(distributing)Instruction counts unchanged — nothing executable is touched.
Flat across a 25× range in membership, because the stanza carries one
<enc type="skmsg">for the whole group and nothing per recipient (pinned separately bywarm_group_send_encoding_scale, #1281). The DM row is higher because a DM pairwise-encrypts once per recipient device.Checked and not changed
Four claims from the first revision were withdrawn after review; each is now documented as a limit rather than a finding:
ensure_sessions_for_devices." Withdrawn — false for the ordinary linked account.src/send/mod.rsstates in theinitial_targetsmatch that own devices are never memoized warm, so own-only SKDM needs is the warm steady state: such a send carries a nonemptydistribution_liston every message. The 22 figure is the zero-own-target case, and the doc now labels it that way.setup_group_sendcallsestablish_sessionfor every member before forcing distribution, soensure_sessions_for_devicesfinds each session present and never reaches the prekey-fetch branch. The figure is the SKDM encrypt fan-out plus stanza build (~26.6 allocations per target). No cold-session cost is measured anywhere in this repository, and the doc says so rather than implying one.resolve_skdm_targets_memoizedminimizes redistribution." Withdrawn — it memoizes device-set resolution. Which devices still need the key is decided byfilter_skdm_targetsagainst theSenderKeyDeviceMap(device_and_primary_warm). The doc now points at that map instead.resolve_skdm_targets_memoizeduntouched in code, per the brief.AllocProfilerdeliberately not committed tosend_receive_benchmark.rs— swapping the global allocator shifts the timing of every bench in that file and would step every one of their CodSpeed series for a number wanted occasionally.voip_benchmark.rsandprekey_store_benchmark.rscarry it because there the churn is the thing under test. The doc carries the three lines to add it back.Validation
--profile benchbuild ofwacore'ssend_receive_benchmark,taskset-pinned, 50 samples per bench.mainoutside the doc: theAllocProfilerline was reverted after measuring.src/send/mod.rsinitial_targetsmatch andfilter_skdm_targets;setup_group_sendin the bench;run_group_send's call graph).