perf!: immutable load_sender_key and auto-reserve marshal

[jlucaso1]

Summary

BREAKING: SenderKeyStore::load_sender_key(&self)

Changes the trait method from &mut self to &self. All 4 implementations verified — none mutate during load:

• Device::load_sender_key — only calls backend.get_sender_key() (read)
• SenderKeyAdapter::load_sender_key — acquires read lock + cache read
• MemSenderKeyStore (bench) — HashMap::get() only
• InMemorySenderKeyStore (bench) — HashMap::get() only

Test callers that held write locks just for load_sender_key are downgraded to read locks.

group_encrypt, group_decrypt, and process_sender_key_distribution_message keep &mut dyn SenderKeyStore because they call store_sender_key (write).

Concurrency unlock: Code that only reads sender keys can now hold shared &self references, enabling concurrent reads at the application level.

marshal_auto_to_vec

New public function in wacore-binary that combines auto-reserve capacity estimation with the fast VecByteWriter path. send_node() now uses this instead of marshal_to (generic Write trait).

Benchmarked (SKDM 256): -0.33% instructions, -2.13% RAM hits — eliminates buffer reallocations for large group stanzas (~50KB).

Test plan

• cargo fmt --all
• cargo clippy --all --tests — zero warnings
• cargo test --workspace --exclude e2e-tests — all 28 suites pass
• Benchmarked with iai-callgrind before/after

Summary by CodeRabbit

• Refactor
  • Simplified message serialization error handling with expression-based control flow.
  • Made sender key store interface more flexible by relaxing borrowing requirements, enabling shared references for key lookups.
  • Updated test infrastructure for improved concurrent access patterns during sender key retrieval.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e04d785f-5cc8-4dc1-96a0-a8b14680abba

📥 Commits

Reviewing files that changed from the base of the PR and between c96401b and eec2830.

📒 Files selected for processing (1)

• src/client.rs

---

📝 Walkthrough

Walkthrough

The PR makes the SenderKeyStore::load_sender_key method callable without mutable access by changing its receiver from &mut self to &self across the trait definition, all implementations, and tests. Additionally, client serialization error handling is refactored to use the ? operator, and test code is optimized to use read locks instead of write locks.

Changes

Cohort / File(s)	Summary
SenderKeyStore Trait & Implementations wacore/libsignal/src/protocol/storage/traits.rs, src/store/signal.rs, src/store/signal_adapter.rs	Updated load_sender_key method signature to take &self instead of &mut self, enabling immutable-reference calls while preserving all behavior.
Benchmark Implementations wacore/benches/send_receive_benchmark.rs, wacore/libsignal/benches/libsignal_benchmark.rs	Updated in-memory SenderKeyStore implementations for benchmarks to use &self receiver for load_sender_key, matching trait signature changes.
Client Serialization src/client.rs	Refactored stanza serialization error handling in Client::send_node from early-return pattern to expression-based ? operator with marshal_auto, maintaining identical error logging and types.
Test Synchronization src/message.rs	Optimized sender-key lookup tests to acquire device guard with read().await instead of write().await, allowing concurrent read access without functional changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• PR #460: Changes callers to use read-lock/cache for sender-key access, complementing this PR's trait borrowing relaxation.
• PR #501: Updates the same SenderKeyStore::load_sender_key API signature and in-memory benchmark implementations.
• PR #415: Modifies the Signal SenderKeyStore API and implementations in parallel with this receiver-borrowing change.

Suggested reviewers

• Ari4ka

Poem

🐰 A sender key that needs no locks,
Immutable reads across the blocks,
No &mut required, just &self will do,
The rabbit's refactor shines brand new! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes two main changes: making load_sender_key immutable and optimizing marshal behavior with auto-reserve, both of which are substantive performance-oriented improvements documented in the PR objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch perf/sender-key-read-and-marshal-auto

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[jlucaso1]

@claude review

[claude]

Claude finished @jlucaso1's task in 1m 22s —— View job

---

Code Review: perf: immutable load_sender_key and auto-reserve marshal

• Read changed files and understand the diff
• Review load_sender_key trait change (&mut self → &self)
• Review marshal_auto_to_vec addition
• Review usage in client.rs
• Post review feedback

---

Verdict: LGTM — clean, minimal, well-scoped changes. Two minor observations below.

SenderKeyStore::load_sender_key (&mut self → &self)

Correct change. The trait method is a pure read operation across all implementations:

• Device — delegates to backend.get_sender_key() (read-only)
• SenderKeyAdapter — read lock + cache lookup
• Both bench stores — HashMap::get() only

The asymmetry with store_sender_key (still &mut self) is appropriate — it's the only method that writes. This matches the pattern already used by SessionStore::load_session(&self) and IdentityKeyStore::get_identity(&self) in the same traits file (wacore/libsignal/src/protocol/storage/traits.rs:134, :88), so the API is now more internally consistent.

marshal_auto_to_vec

The function at wacore/binary/src/marshal.rs:45-50 is a clean composition of existing primitives (should_auto_reserve_node + estimate_capacity_node + marshal_to_vec). No issues.

One minor note: send_node at src/client.rs:3417 creates a Vec::with_capacity(1024), and then marshal_auto_to_vec may call output.reserve(estimate) on top of that. For small nodes that don't trigger auto-reserve, the 1024 pre-allocation is fine. For large nodes, the estimate will dominate and the initial 1024 capacity becomes a no-op (since reserve checks existing capacity). So no wasted allocation — this is correct.

Nit (non-blocking)

The existing marshal_auto function (marshal.rs:62) creates a fresh Vec internally. If send_node were refactored to just call marshal_auto(&node)? instead of managing its own buffer, it would be slightly simpler — but the current approach avoids an extra allocation when send_node is called in a hot loop, since the buffer could potentially be hoisted out and reused in the future. Fine as-is.

---

[github-actions]

Bencher Report

Branch	perf/sender-key-read-and-marshal-auto
Testbed	ubuntu-latest

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🚨 1 Alert

🐰 View full continuous benchmarking report in Bencher