perf: lazy ciphertext in SignalMessage, eliminate redundant allocation - #510
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSignalMessage now keeps canonical serialized bytes and lazily decodes/caches ciphertext in a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/libsignal/src/protocol/protocol.rs`:
- Around line 159-184: The body() method currently handles the OnceLock path
then calls decode_ciphertext() and sets the cache but still treats a subsequent
get() failure as a reachable error; simplify to mirror
SenderKeyMessage::ciphertext() by matching on self.ciphertext_cache.get(): if
Some(ct) return Ok(ct.as_ref()); if None, call decode_ciphertext() to produce
Box<[u8]>, set it into ciphertext_cache, then return the cached value
(unwrapping the get() result because a race loser here is unreachable).
Reference: body(), decode_ciphertext(), and ciphertext_cache (compare to
SenderKeyMessage::ciphertext()).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d96ed179-b78b-4e96-a349-5702d5956eed
📒 Files selected for processing (3)
wacore/libsignal/benches/libsignal_benchmark.rswacore/libsignal/src/protocol/protocol.rswacore/libsignal/src/protocol/session_cipher.rs
Benchmark Results59 unchanged benchmark(s)
|
7d7d7b3 to
aad011c
Compare
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 26s —— View job Code Review: perf: lazy ciphertext in SignalMessage
Verdict: Looks good. Clean, well-motivated optimization that follows the established Strengths
Minor observation (not blocking)
|
aad011c to
e123a05
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/libsignal/src/protocol/protocol.rs (1)
531-534: 🧹 Nitpick | 🔵 TrivialConsider aligning
SenderKeyMessage::ciphertext()with theSignalMessage::body()pattern.
SignalMessage::body()now usesexpect("just set")(line 165) which is cleaner than the match pattern here. The error branch at line 533 is unreachable for the same reason — afterset(), the value is always present.♻️ Optional: Align with SignalMessage::body() pattern
let ciphertext = self.decode_ciphertext()?; let _ = self.ciphertext_cache.set(ciphertext); - match self.ciphertext_cache.get() { - Some(ciphertext) => Ok(ciphertext.as_ref()), - None => Err(SignalProtocolError::InvalidProtobufEncoding), - } + // get() can't be None: even if a racing set() lost, the winner's value is stored + Ok(self.ciphertext_cache.get().expect("just set").as_ref())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/protocol.rs` around lines 531 - 534, SenderKeyMessage::ciphertext() currently matches on self.ciphertext_cache.get() and returns an unreachable Err branch; update it to follow SignalMessage::body() by calling self.ciphertext_cache.get().expect("just set") and returning its as_ref() directly so the unreachable error path is removed and behavior is consistent with SignalMessage::body().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wacore/libsignal/src/protocol/protocol.rs`:
- Around line 531-534: SenderKeyMessage::ciphertext() currently matches on
self.ciphertext_cache.get() and returns an unreachable Err branch; update it to
follow SignalMessage::body() by calling self.ciphertext_cache.get().expect("just
set") and returning its as_ref() directly so the unreachable error path is
removed and behavior is consistent with SignalMessage::body().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e416ac68-c1b4-4698-8630-6ef0292ed6a1
📒 Files selected for processing (2)
wacore/libsignal/src/protocol/protocol.rswacore/libsignal/src/protocol/session_cipher.rs
e123a05 to
45f4530
Compare
SignalMessage stored the ciphertext body in two places: a dedicated `ciphertext: Box<[u8]>` field AND embedded inside the `serialized` protobuf wire bytes. On the send path, `.body()` is never called, so the dedicated field was dead weight — one extra heap allocation + memcpy per sent message. Replace with `OnceLock<Box<[u8]>>` (same pattern SenderKeyMessage already uses). On the send path the cache stays empty (44% less struct heap memory). On the receive path, TryFrom eagerly populates the cache so `.body()` is still a simple pointer return.
45f4530 to
c329759
Compare
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 17s —— View job Code Review: perf: lazy ciphertext in SignalMessage
Verdict: LGTM. Clean optimization that unifies two code paths and removes a redundant allocation. No issues found. What this PR does well
One minor observation (not blocking)The No issues, no regressions, no further changes needed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/libsignal/src/protocol/protocol.rs`:
- Around line 170-181: Change SignalMessage::body to be infallible: make pub fn
body(&self) -> &[u8] (not Result) and have it return the cached bytes by calling
the init helper and unwrapping safely based on the internal invariant (e.g., use
get_or_try_init_bytes(...).expect("ciphertext must be present") or switch to a
get_or_init_bytes helper if available) so decode_ciphertext can remain returning
Box<[u8]>; keep ciphertext_cache seeding behavior in TryFrom/new intact and then
update callers (like session_cipher.rs) to drop the unnecessary `?` since body
no longer returns Result.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c7b55cd-a4c3-4ef1-9484-d8c29882405f
📒 Files selected for processing (2)
wacore/libsignal/src/protocol/protocol.rswacore/libsignal/src/protocol/session_cipher.rs
| pub fn body(&self) -> Result<&[u8]> { | ||
| get_or_try_init_bytes(&self.ciphertext_cache, || self.decode_ciphertext()) | ||
| } | ||
|
|
||
| fn decode_ciphertext(&self) -> Result<Box<[u8]>> { | ||
| let proto_bytes = &self.serialized[1..self.serialized.len() - Self::MAC_LENGTH]; | ||
| let proto = waproto::whatsapp::SignalMessage::decode(proto_bytes) | ||
| .map_err(|_| SignalProtocolError::InvalidProtobufEncoding)?; | ||
| proto | ||
| .ciphertext | ||
| .ok_or(SignalProtocolError::InvalidProtobufEncoding) | ||
| .map(|v| v.into_boxed_slice()) |
There was a problem hiding this comment.
Keep SignalMessage::body() infallible.
All valid SignalMessage instances already guarantee a ciphertext: new() serializes ciphertext: Some(...), and TryFrom<&[u8]> rejects missing ciphertext while eagerly seeding ciphertext_cache in Lines 239-279. Exposing Result<&[u8]> here turns an internal invariant into a public breaking change and forces avoidable churn into callers like session_cipher.rs for no reachable recovery path.
♻️ Suggested direction
- pub fn body(&self) -> Result<&[u8]> {
- get_or_try_init_bytes(&self.ciphertext_cache, || self.decode_ciphertext())
+ pub fn body(&self) -> &[u8] {
+ if let Some(ct) = self.ciphertext_cache.get() {
+ return ct.as_ref();
+ }
+
+ let ct = self
+ .decode_ciphertext()
+ .expect("SignalMessage invariant violated: missing ciphertext");
+ let _ = self.ciphertext_cache.set(ct);
+ self.ciphertext_cache.get().expect("just set").as_ref()
}// Then drop the `?` at the `ciphertext.body()` call site in
// `wacore/libsignal/src/protocol/session_cipher.rs`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/libsignal/src/protocol/protocol.rs` around lines 170 - 181, Change
SignalMessage::body to be infallible: make pub fn body(&self) -> &[u8] (not
Result) and have it return the cached bytes by calling the init helper and
unwrapping safely based on the internal invariant (e.g., use
get_or_try_init_bytes(...).expect("ciphertext must be present") or switch to a
get_or_init_bytes helper if available) so decode_ciphertext can remain returning
Box<[u8]>; keep ciphertext_cache seeding behavior in TryFrom/new intact and then
update callers (like session_cipher.rs) to drop the unnecessary `?` since body
no longer returns Result.
1 participant
Summary
ciphertext: Box<[u8]>field fromSignalMessage— it stored the same bytes already embedded inserializedOnceLock<Box<[u8]>>lazy cache (mirrors existingSenderKeyMessagepattern).body()is never called, so the cache stays empty — eliminates 1 heap allocation + memcpy of N ciphertext bytes per sent message (44% less struct heap memory)TryFromeagerly populates the cache, so.body()is still a zero-cost pointer returnSignalMessage::body()return type changes from&[u8]toResult<&[u8]>(only caller updated insession_cipher.rs)Benchmark results
No measurable CPU regression on any real path.
Test plan
cargo fmt --all— cleancargo clippy --all --tests— zero warningscargo test -p wacore-libsignal— 102 tests passcargo test -p whatsapp-rust— 345 tests passcargo bench -p wacore-libsignal --bench libsignal_benchmark— no regressionsSummary by CodeRabbit
Bug Fixes
Refactor