perf: 11 allocation trims from hot-path audit - #570
Merged
Conversation
Audit pass across the send, receive, retry, and signal paths — pattern matches the same shape as the previous PR's win (eager to_non_ad() / clone() then discarded, map without with_capacity, clone for consumer that only borrows). No behavior change in any of the spots. Per-message / per-receive: - message.rs::parse_message_info: stop cloning the entire `Device` just to read .pn and .lid. Read the two fields through the RwLock guard directly, mirroring the get_pn() getter pattern. - message.rs (batch skmsg decrypt loop): hoist sender_for_sk, sender_address, and sender_key_name out of `for payload in payloads` — all three are loop-invariant. Saves 3 allocations per extra payload in a batch (common for group status). Per-send: - send.rs (DM fanout): replace the `HashSet::insert(j.clone())` dedup-retain with `sort_dedup_by_device`. participant_list_hash sorts internally, so reordering is safe; now zero allocations for dedup. - wacore/src/send.rs (force-SKDM distribution): don't pre-allocate own_jid_to_check before checking presence in the list — compare by `&str` user, allocate only on the push branch. Saves 1 Jid per send on the "own already present" path (the common case). Per-retry: - retry.rs: drop the second processing_key.clone() — move it into the scopeguard closure instead of cloning twice. - retry.rs: replace `info.requester.user.clone()` with a direct borrow where the only consumer is `has_device(&str)`. - retry.rs: turn `recipient.clone().unwrap().to_non_ad()` into `recipient.as_ref()` + pattern match. Getters: - client.rs: get_push_name and get_lid were cloning the whole device_snapshot. Match get_pn: read the one field via persistence_manager.get_device_arc().read().await.<field>.clone(). - sender_keys.rs: borrow `own_lid_user` / `own_pn_user` as `&str` into the filter closure instead of cloning each `CompactString` up-front — the snapshot stays alive for the .collect() call. Other: - features/groups.rs::query_info: collapse two iterations of `group.participants` into a single move-based loop — saves one Jid clone per participant, plus one CompactString clone per LID entry with a PN mapping. - features/polls.rs::vote: replace `my_jid.to_non_ad() == poll_creator_jid.to_non_ad()` with `is_same_user_as` (the same fix shape from the reference PR). Cache `my_base = my_jid.to_non_ad()` so the voter_jid_str derivation and the equality check share one allocation. - features/polls.rs::aggregate_votes: inline the decryption helper to cache `creator_str` once instead of recomputing `poll_creator_jid.to_non_ad().to_string()` per voter. Added HashMap::with_capacity(votes.len()) while we're here. - wacore/src/prekeys.rs::parse_prekeys_response: HashMap::with_capacity(children.len()). - sender_key_device_cache.rs::from_db_rows: HashMap::with_capacity(rows.len()), HashSet ditto. 552 wacore lib tests + all per-crate suites remain green.
📝 WalkthroughSummary by CodeRabbit
Note: These are internal improvements with no visible changes to user-facing functionality. WalkthroughThis PR refactors data access patterns across multiple modules to optimize state management and loop iterations. Changes include replacing snapshot cloning with RwLock-guarded shared state access, precomputing invariant values outside loops, simplifying tuple constructions, and leveraging wacore helper functions for deduplication. No public API signatures are altered. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/client.rs`:
- Around line 3584-3590: The code is reading Device state via
self.persistence_manager.get_device_arc().read().await which violates the read
contract; change both occurrences (the getter that accesses push_name and the
other at lines ~3604-3610) to use
self.persistence_manager.get_device_snapshot().await so reads use the snapshot
accessor, and keep any mutations routed through DeviceCommand +
PersistenceManager::process_command(); locate uses of get_device_arc(), replace
with get_device_snapshot().await (or an approved accessor) and ensure no direct
Device state mutation occurs outside DeviceCommand/ProcessCommand.
In `@src/message.rs`:
- Around line 1101-1107: The code is incorrectly stripping device scope by
calling to_non_ad() on info.source.sender before generating the sender key name,
which can cause missing device-scoped sender-key state; remove the to_non_ad()
call and build the protocol address from the original device-scoped sender (use
info.source.sender.to_protocol_address()) so that sender_address and the call to
make_sender_key_name(&info.source.chat, &sender_address) use the device-scoped
LID protocol address instead of a non-AD normalized one.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 813a0e67-8f1d-4a9e-8855-2d8107818a8b
📒 Files selected for processing (10)
src/client.rssrc/client/sender_keys.rssrc/features/groups.rssrc/features/polls.rssrc/message.rssrc/retry.rssrc/send.rssrc/sender_key_device_cache.rswacore/src/prekeys.rswacore/src/send.rs
Comment on lines
+3584
to
+3590
| self.persistence_manager | ||
| .get_device_arc() | ||
| .await | ||
| .read() | ||
| .await | ||
| .push_name | ||
| .clone() |
There was a problem hiding this comment.
We need to keep device reads on the snapshot path
Line 3584 and Line 3604 now read through get_device_arc().read(). That breaks the repo’s state-access contract for reads. Move both getters back to get_device_snapshot().await (or an explicitly approved accessor that preserves the same contract).
Suggested fix
pub async fn get_push_name(&self) -> String {
self.persistence_manager
- .get_device_arc()
- .await
- .read()
+ .get_device_snapshot()
.await
.push_name
.clone()
}
@@
pub async fn get_lid(&self) -> Option<Jid> {
self.persistence_manager
- .get_device_arc()
- .await
- .read()
+ .get_device_snapshot()
.await
.lid
.clone()
}As per coding guidelines: Never modify Device state directly; always use DeviceCommand + PersistenceManager::process_command() for state mutations and get_device_snapshot() for reading state.
Also applies to: 3604-3610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client.rs` around lines 3584 - 3590, The code is reading Device state via
self.persistence_manager.get_device_arc().read().await which violates the read
contract; change both occurrences (the getter that accesses push_name and the
other at lines ~3604-3610) to use
self.persistence_manager.get_device_snapshot().await so reads use the snapshot
accessor, and keep any mutations routed through DeviceCommand +
PersistenceManager::process_command(); locate uses of get_device_arc(), replace
with get_device_snapshot().await (or an approved accessor) and ensure no direct
Device state mutation occurs outside DeviceCommand/ProcessCommand.
Comment on lines
+1101
to
+1107
| // Always use bare sender for sender key operations. Real WA delivers | ||
| // skmsg with bare participant but pkmsg (SKDM) with device-qualified | ||
| // participant — normalizing to bare ensures consistent lookup. | ||
| // Hoisted out of the payload loop: all three are loop-invariant. | ||
| let sender_for_sk = info.source.sender.to_non_ad(); | ||
| let sender_address = sender_for_sk.to_protocol_address(); | ||
| let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address); |
There was a problem hiding this comment.
Stop stripping device scope from sender-key addresses here.
This path normalizes info.source.sender with to_non_ad(), which can collapse device-qualified LID sender identities and miss sender-key state keyed by the device-scoped address.
Suggested fix
- let sender_for_sk = info.source.sender.to_non_ad();
- let sender_address = sender_for_sk.to_protocol_address();
+ let sender_address = info.source.sender.to_protocol_address();
let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address);Based on learnings: sender-key naming in this codebase should use device-scoped LID protocol addresses (to_protocol_address()), not to_non_ad().to_protocol_address().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Always use bare sender for sender key operations. Real WA delivers | |
| // skmsg with bare participant but pkmsg (SKDM) with device-qualified | |
| // participant — normalizing to bare ensures consistent lookup. | |
| // Hoisted out of the payload loop: all three are loop-invariant. | |
| let sender_for_sk = info.source.sender.to_non_ad(); | |
| let sender_address = sender_for_sk.to_protocol_address(); | |
| let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address); | |
| // Always use bare sender for sender key operations. Real WA delivers | |
| // skmsg with bare participant but pkmsg (SKDM) with device-qualified | |
| // participant — normalizing to bare ensures consistent lookup. | |
| // Hoisted out of the payload loop: all three are loop-invariant. | |
| let sender_address = info.source.sender.to_protocol_address(); | |
| let sender_key_name = make_sender_key_name(&info.source.chat, &sender_address); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 1101 - 1107, The code is incorrectly stripping
device scope by calling to_non_ad() on info.source.sender before generating the
sender key name, which can cause missing device-scoped sender-key state; remove
the to_non_ad() call and build the protocol address from the original
device-scoped sender (use info.source.sender.to_protocol_address()) so that
sender_address and the call to make_sender_key_name(&info.source.chat,
&sender_address) use the device-scoped LID protocol address instead of a non-AD
normalized one.
Benchmark Results59 unchanged benchmark(s)
|
3 tasks
This was referenced Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Follow-up audit sweep after PR #569 surfaced two reference wins (
assemble_status_participants+resolve_skdm_targets). A background agent mapped the same class of patterns across the rest of the workspace; 11 of those hits landed here. Every finding is one of:.to_non_ad()/.clone()followed by a branch that discards it.HashMap::new()where capacity is knowable.No behavior change. All wins are mechanical.
Findings, by hot-path frequency
Per inbound message
src/message.rs::parse_message_infoDeviceto readpn+lid; read via the RwLock guard, mirroringget_pn(). Saves a fullDeviceclone (CompactStrings +Jids + keys +AdvSignedDeviceIdentity) per stanza.src/message.rs(batchskmsgdecrypt loop)sender_for_sk/sender_address/sender_key_nameout offor payload in payloads— all three are loop-invariant. Saves 3 allocations per extra payload in a batch (common for group / status fanouts).Per send
src/send.rs(DM fanout dedup)HashSet::insert(j.clone())retain withsort_dedup_by_device.participant_list_hashsorts internally, so reordering is safe — zero allocations now.wacore/src/send.rs(force-SKDM distribution)own_jid_to_checkbefore the presence check; compare by&struser, allocate only on the push branch. Saves oneJidon the common "own already in list" path.Per retry
src/retry.rs(processing key)processing_key.clone()— move it into the scopeguard closure instead.src/retry.rs(sender_user)info.requester.user.clone()with a direct borrow; the only consumer ishas_device(&str).src/retry.rs(recipient chat)recipient.clone().unwrap().to_non_ad()with an.as_ref()pattern match.Public getters
src/client.rs::get_push_name/get_lidpersistence_manager.get_device_arc().read().await.<field>.clone()likeget_pn()already does.src/client/sender_keys.rs::set_sender_key_status_for_devicesown_lid_user/own_pn_useras&strinto the filter closure; snapshot stays alive via a binding for the duration of.collect().Group / poll paths
src/features/groups.rs::query_infogroup.participantsinto a single move-based loop. Saves oneJidclone per participant plus oneCompactStringper LID entry with a PN mapping.src/features/polls.rs::votemy_jid.to_non_ad() == poll_creator_jid.to_non_ad()withis_same_user_as(the reference-PR shape). Cachemy_base = my_jid.to_non_ad()to share one allocation between the voter string and the equality check.src/features/polls.rs::aggregate_votescreator_str = poll_creator_jid.to_non_ad().to_string()is computed once instead of per voter. Pre-allocatelatest_voteswithvotes.len()capacity.Map capacity
wacore/src/prekeys.rs::parse_prekeys_responseHashMap::with_capacity(children.len()). Rehash was possible up to N users per prekey fetch.src/sender_key_device_cache.rs::from_db_rowsHashMap::with_capacity(rows.len())+HashSet::with_capacity(rows.len() / 4).Verification
cargo fmt --allcargo clippy --all --tests --exclude e2e-tests— cleancargo test --workspace --exclude e2e-tests --exclude bench-integration— 552 wacore lib tests + all per-crate suites greenNot included
Flagged by the audit but left alone pending human review or verified load-bearing:
wacore/src/store/signal_cache.rsflush uses.iter().cloned().collect()three times per flush;drain()would avoid the Arc bumps but changes failure semantics (current code clears only successful writes) — needs intent confirmation.wacore/src/iq/usync.rs:429(fields.jid.clone()),src/portable_cache.rs:139((k, _)|k.clone()),wacore/src/appstate_sync.rs:35((**arc).clone()) — verified necessary and/or cold paths.