perf!: Arc<MessageInfo> across message, retry, and PDO paths - #520
Conversation
📝 WalkthroughWalkthroughConvert message metadata to shared ownership by using 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 |
|
@claude review |
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 `@src/message.rs`:
- Around line 123-128: The retry path still clones MessageInfo deeply because
spawn_retry_receipt accepts an owned MessageInfo; change spawn_retry_receipt's
signature to take Arc<MessageInfo> (or &Arc<MessageInfo>) and update its
implementation to use the Arc without cloning the inner MessageInfo, then update
this caller in the retry helper to pass Arc::clone(info) (or the reference)
instead of a deep clone; adjust any other callers of spawn_retry_receipt to
match the new signature and remove the unnecessary MessageInfo::clone
allocations.
🪄 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: 75124258-97e7-4d06-a48f-fb279c521760
📒 Files selected for processing (4)
Cargo.tomlsrc/message.rssrc/pdo.rswacore/src/types/events.rs
|
Claude finished @jlucaso1's task in 3m 12s —— View job Code Review:
|
Benchmark Results59 unchanged benchmark(s)
|
- Event::Message and UndecryptableMessage now hold Arc<MessageInfo> - dispatch_parsed_message: conditionally clones only when ephemeral mutation is needed, otherwise Arc::clone (zero deep clones on the common path) - spawn_retry_receipt: takes &Arc<MessageInfo>, Arc::clone instead of deep clone for the spawned async task - Thread &Arc<MessageInfo> through the internal message processing chain (process_session_enc_batch, process_group_enc_batch, handle_decrypted_plaintext, try_pn_to_lid_migration_decrypt, handle_decrypt_failure) to avoid re-cloning at each layer - Enable serde "rc" feature for Arc<T> serialization
f1d00e2 to
f97e2bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/message.rs`:
- Around line 41-49: The code currently deep-clones the MessageInfo via
(**info).clone() when setting ephemeral_expiration; instead perform
copy-on-write with Arc::make_mut to avoid the clone when the Arc is uniquely
owned: clone the Arc with Arc::clone(info) into a mutable binding (e.g., let mut
info = Arc::clone(info)), then inside the branch call Arc::make_mut(&mut info)
to get &mut MessageInfo and set ephemeral_expiration to
msg.get_base_message().get_ephemeral_expiration(); otherwise leave the cloned
Arc untouched and continue using info. This replaces the (**info).clone() path
while preserving semantics.
In `@src/pdo.rs`:
- Around line 374-376: PendingPdoRequest.message_info and
Client::send_pdo_placeholder_resend_request currently use owned MessageInfo so
you allocate a new Arc in the fallback dispatch; change
PendingPdoRequest.message_info to Arc<MessageInfo> and update all construction
sites to wrap or pass an Arc, then change
Client::send_pdo_placeholder_resend_request signature to take Arc<MessageInfo>
(or &Arc<MessageInfo>) and propagate that Arc through the code so you can reuse
Arc::clone rather than calling Arc::new in the dispatch (replace
Arc::new(message_info) with the existing Arc instance or Arc::clone). Ensure all
call sites and tests that previously passed MessageInfo are updated to either
pass an Arc or convert once at creation points.
🪄 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: 9f0c99c5-f2b9-4c77-8ce6-36640eb621d3
📒 Files selected for processing (4)
Cargo.tomlsrc/message.rssrc/pdo.rswacore/src/types/events.rs
- dispatch_parsed_message: use Arc::make_mut for ephemeral mutation instead of deep clone + Arc::new - PendingPdoRequest.message_info: Arc<MessageInfo> instead of owned - spawn_pdo_request / spawn_pdo_request_with_options: take &Arc<MessageInfo>, Arc::clone instead of deep clone - send_pdo_placeholder_resend_request: take &Arc<MessageInfo> - handle_pdo_response: Arc::make_mut for mutations, pass Arc directly to Event dispatch without re-wrapping
Summary
Eliminates all
MessageInfodeep clones in the message processing pipeline by threadingArc<MessageInfo>through every layer.Event types:
Event::MessageandUndecryptableMessagenow holdArc<MessageInfo>Event::as_message()still returns(&wa::Message, &MessageInfo)via Arc derefMessage dispatch:
dispatch_parsed_message: usesArc::make_mutfor ephemeral mutation — zero deep clones when no mutation needed (common path), in-place mutation when Arc is uniquely ownedspawn_retry_receipt: takes&Arc<MessageInfo>,Arc::cloneinstead of deep clonePDO path:
PendingPdoRequest.message_info:Arc<MessageInfo>instead of ownedspawn_pdo_request/spawn_pdo_request_with_options/send_pdo_placeholder_resend_request: take&Arc<MessageInfo>,Arc::cloneinstead of deep clonehandle_pdo_response:Arc::make_mutfor mutations, passes Arc directly to event dispatchInternal chain:
process_session_enc_batch,process_group_enc_batch,handle_decrypted_plaintext,try_pn_to_lid_migration_decrypt,handle_decrypt_failure,handle_newsletter_messageall take&Arc<MessageInfo>to avoid re-cloning at each layerSerde:
rcfeature forArc<T>serializationBreaking changes
Event::Message(Box<wa::Message>, MessageInfo)→Event::Message(Box<wa::Message>, Arc<MessageInfo>)UndecryptableMessage.info: MessageInfo→Arc<MessageInfo>PendingPdoRequest.message_info: MessageInfo→Arc<MessageInfo>Before / After
(*arc).clone())Test plan
cargo fmt --allcargo clippy --all --testscargo test --all --exclude e2e-tests(724 tests pass)