docs: update ProtocolAddress and session locking for inline-SSO refactor (whatsapp-rust#1131) - #447
Conversation
…tor (whatsapp-rust#1131) whatsapp-rust PR #1131 replaced ProtocolAddress's single-String buffer with an inline-up-to-47-bytes small-buffer optimization, changed `new()` to take `&str`, removed `with_capacity()` in favor of `empty()`, and replaced the resolve-then-lock `session_mutexes_for()` pattern with `session_guards_for()`, which locks each mutex as it is resolved. Update the affected reference docs to match the new APIs and drop the now-inaccurate allocation-count claims.
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
| Filename | Overview |
|---|---|
| advanced/signal-protocol.mdx | Documents the new address-buffer representation and session guard flow; the previous allocation-savings overstatement is corrected to at most 99 avoided allocations. |
| api/signal.mdx | Updates the documented group participant encryption lock helper to session_guards_for(). |
| concepts/architecture.mdx | Aligns the concurrency overview with immediate, ordered lock acquisition through session_guards_for(). |
| concepts/storage.mdx | Corrects the session-key allocation explanation for inline ProtocolAddress storage. |
Reviews (2): Last reviewed commit: "docs: correct reused-buffer allocation s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The first spill on a reused ProtocolAddress still allocates its backing String; only subsequent resets avoid a new allocation. Addressed via review feedback on #447.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: 04be73d33f
ℹ️ 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".
| ``` | ||
|
|
||
| `to_protocol_address_string()` is used on hot paths (message encryption and decryption) as the key for `session_locks`. It pre-sizes the output buffer and builds the string in a single allocation, avoiding the two-allocation overhead of constructing a `ProtocolAddress` and then calling `.to_string()`. | ||
| `to_protocol_address_string()` is used on hot paths (message encryption and decryption) as the key for `session_locks`. It pre-sizes the output buffer and builds the `String` in a single allocation. Constructing a `ProtocolAddress` itself no longer allocates for addresses that fit inline (see [Single-buffer ProtocolAddress](#single-buffer-protocoladdress) below), but `.to_string()` on top of it still does, so `to_protocol_address_string()` remains the cheaper path when only the string is needed. |
There was a problem hiding this comment.
This still says the hot-path session_locks key is produced by to_protocol_address_string(), but the inline-SSO refactor uses reusable ProtocolAddress values as the lock-cache keys; otherwise session_guards_for() could not avoid constructing a string key. The same stale model remains in concepts/architecture.mdx, which declares Cache<String, ...>, and in concepts/storage.mdx. Update these descriptions and snippets to the new key type so readers are not given the pre-#1131 allocation and cache behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This premise doesn't hold — I checked against whatsapp-rust@main:
session_locks is still Cache<String, Arc<Mutex<()>>> (src/client.rs:1000), unchanged by #1131. session_lock_for(signal_addr_str: &str) (src/client/adapters.rs:38) calls self.session_locks.get_with_by_ref(signal_addr_str, ...) — get_with_by_ref only allocates an owned String key on a cache miss; on a hit it takes &str and allocates nothing. That's what lets session_guards_for() pass addr.as_str() (from a reused ProtocolAddress) without constructing a string key, with no need for the cache itself to be keyed by ProtocolAddress.
This isn't new either — get_with_by_ref already existed at #1131's base commit (1118084), so it predates the inline-SSO refactor entirely. The docs' Cache<String, ...> declaration in concepts/architecture.mdx and the to_protocol_address_string() description in advanced/signal-protocol.mdx are accurate as written. No change needed here.
Generated by Claude Code
2 participants
Summary
Reflects whatsapp-rust#1131 ("perf: five cuts to the per-message allocation count"), which made two internal changes that the docs described inaccurately:
ProtocolAddresswent from a single-Stringbuffer toAddressBuf, a small-buffer optimization that keeps addresses up to 47 bytes inline with zero heap allocation, spilling to aStringbeyond that.ProtocolAddress::new()now takes&strinstead of an ownedString,with_capacity(capacity, device_id)was removed in favor ofempty(device_id)(no capacity hint needed), andreset_with()'s closure now receives&mut AddressBufinstead of&mut String.session_mutexes_for()(resolve every mutex, then lock each in a second pass) is now#[cfg(test)]-only. Production code uses the newsession_guards_for(), which locks each mutex as it is resolved.Changes
advanced/signal-protocol.mdx: rewrote the "Single-buffer ProtocolAddress" section for the inline/spill design (including theDebug-leak safety note the PR fixed), updated the "Reusable hot-loop address construction" and "Single-allocation session lock keys" sections and code samples forempty()/session_guards_for(), and corrected two now-stale allocation-count claims.api/signal.mdx,concepts/architecture.mdx: updated references tosession_mutexes_for()→session_guards_for().concepts/storage.mdx: corrected the "two-allocation overhead" claim, sinceProtocolAddressconstruction is now allocation-free for inline-sized addresses.Changelog entries are intentionally left untouched per project convention (human-authored only).
Test plan
wacore/libsignal/src/core/address.rsandwacore/src/types/jid.rsonwhatsapp-rust@mainsession_mutexes_for()/with_capacityoutside test-only context — none foundmint broken-links(not run — Mintlify CLI unavailable in this environment; no new headings were added, only existing anchors reused)Generated by Claude Code
Summary by cubic
Updates Signal Protocol docs to match
whatsapp-rustPR#1131:ProtocolAddressnow uses an inline-first buffer and the send path usessession_guards_for()instead ofsession_mutexes_for(). Also corrects the reused-buffer allocation note for spilled addresses (saves up to 99 allocations, not 100).ProtocolAddress::new(name: &str, device_id)(wasString).with_capacity(...)withempty(device_id).reset_with()closure now takes&mut AddressBuf(was&mut String).session_guards_for(&jids);session_mutexes_for()is test-only.Written for commit 19d7d76. Summary will update on new commits.