perf!: cut clones/allocs in LID resolution, single-device send, history-sync - #690
Merged
Conversation
…ry-sync M1: get_current_lid / SendContextResolver::get_lid_for_phone / IsOnWhatsAppUser.known_lid now return CompactString (inline for typical ~15-digit LIDs) instead of deep-cloning a String from the Arc-shared cache entry; the JID user field is CompactString, so callers building JIDs no longer convert. M3-adjacent C3: a single-recipient send now encrypts inline, skipping the parallel fan-out's Arc<[u8]> plaintext copy, spawned task + oneshot channel, FuturesUnordered, and two store clones (no parallelism is lost with one device); the per-device encrypt + result-handling are factored into shared helpers so both paths behave identically. C5: drop the history-sync pre-count pass (measured -2.5% of decode) - it scanned the whole blob to size a Vec that only holds the secret-record subset, over-allocating; plain growth is cheaper. Breaking: CompactString return types pre-1.0.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR converts LID-related fields and returns from String to wacore_binary::CompactString across trait, cache, client, and usync; refactors wacore send per-device encryption into helpers with a single-recipient fast path; and removes history-sync pre-sizing helpers. ChangesLID CompactString migration and encryption & history refactors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Benchmark Results67 unchanged benchmark(s)
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/send.rs`:
- Around line 507-530: encrypt_one_device is currently cloning and storing
mediatype into every per-device EncryptOneResult, causing unnecessary heap
allocations; remove mediatype from EncryptOneResult and from the
encrypt_one_device signature/return so per-device results only contain enc_type,
is_prekey, ciphertext, and hide_decrypt_fail; update callers (the multi-device
fan-out that awaits encrypt_one_device tasks and the aggregation code that
builds the final <enc> element) to pass or use a single batch-level mediatype
when constructing the <enc> payload instead of relying on per-device results;
ensure functions referenced here (encrypt_one_device, EncryptOneResult,
message_encrypt, extract_ciphertext and the aggregation/fan-out code that
consumes EncryptOneResult) are adjusted accordingly and no longer clone
mediatype per device.
🪄 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: fd8896c9-85be-4b28-8163-05adcc4aac14
📒 Files selected for processing (12)
src/client/context_impl.rssrc/client/device_registry.rssrc/client/lid_pn.rssrc/features/groups.rssrc/handlers/notification.rssrc/lid_pn_cache.rssrc/message.rssrc/send.rswacore/src/client/context.rswacore/src/history_sync.rswacore/src/iq/usync.rswacore/src/send.rs
mediatype is the same &'static str for every device in a send; it was cloned into each per-device EncryptOneResult (and, on the parallel path, cloned again per task). Drop it from EncryptOneResult and encrypt_one_device; thread the batch-level Option<&str> straight into push_encrypt_result when building the <enc> node.
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/src/send.rs (1)
523-528: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider zero-copy conversion from
Box<[u8]>toVec<u8>.Look, this PR is all about cutting allocations, right? But here we're using
.to_vec()which deref-coercesBox<[u8]>to&[u8]and then copies the bytes. For a perf-focused change, that's leaving gains on the table.
Vec::from(box)or.into()leveragesFrom<Box<[T]>> for Vec<T>which reuses the allocation — zero copy.♻️ Proposed fix for zero-copy conversion
( device_jid, Ok(Some(EncryptOneResult { enc_type, is_prekey, - ciphertext: serialized_bytes.to_vec(), + ciphertext: serialized_bytes.into(), hide_decrypt_fail, })), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/send.rs` around lines 523 - 528, The code copies the boxed byte slice when constructing EncryptOneResult.ciphertext via serialized_bytes.to_vec(), which defeats the PR's allocation-reduction goal; replace the copy with a zero-copy conversion using Vec::from(serialized_bytes) or serialized_bytes.into() so the Box<[u8]> allocation is reused when building the EncryptOneResult.ciphertext (ensure serialized_bytes is a Box<[u8]> or convert it to one before the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wacore/src/send.rs`:
- Around line 523-528: The code copies the boxed byte slice when constructing
EncryptOneResult.ciphertext via serialized_bytes.to_vec(), which defeats the
PR's allocation-reduction goal; replace the copy with a zero-copy conversion
using Vec::from(serialized_bytes) or serialized_bytes.into() so the Box<[u8]>
allocation is reused when building the EncryptOneResult.ciphertext (ensure
serialized_bytes is a Box<[u8]> or convert it to one before the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 38a0f6d3-8844-40e4-b731-d72908835887
📒 Files selected for processing (1)
wacore/src/send.rs
extract_ciphertext returns Box<[u8]>; serialized_bytes.to_vec() copied it into a fresh Vec. Vec::from(Box<[u8]>) (via .into()) reuses the existing allocation, so the per-device ciphertext no longer pays an extra copy.
This was referenced Jun 5, 2026
Merged
This was referenced Jul 2, 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.
Three independent allocation/clone cleanups (breaking signature changes are fine pre-1.0).
M1 — LID lookups return
CompactString, not a clonedStringThe LID/PN cache stores
Arc<LidPnEntry>, butget_current_liddeep-cloned the entry'sStringon every call. It (andSendContextResolver::get_lid_for_phone+IsOnWhatsAppUser.known_lid) now returnCompactString, which is inline for the typical ~15-digit LID user — no heap allocation. The JIDuserfield is alreadyCompactString, so the common "build a LID JID from the lookup" callers no longer convert at all. This runs per-recipient during group-send fanout and per-destination on DMs.C3 — single-recipient send skips the parallel fan-out
encrypt_for_devicesalways built anArc<[u8]>copy of the plaintext, spawned a task + oneshot channel per device, and ran aFuturesUnordered— even for one device, where there's no parallelism to gain. A single recipient now encrypts inline. The per-device encrypt and the result-handling are factored into shared helpers (encrypt_one_device,push_encrypt_result) used by both the fast path and the parallel path, so behavior is identical.C5 — drop the history-sync pre-count pass
process_history_sync(retain-blob path) scanned the whole decompressed blob once just to size the secret-recordVec. But it counted messages while the Vec only holds the secret-record subset, so it over-allocated and paid a full extra scan. Plain growth is cheaper.Measured (iai): removing the count pass = −2.5% instructions on a 20k-message decode. M1/C3 are structural alloc/overhead removals (a heap String clone per LID lookup; an Arc copy + task spawn + 2 store clones per single-device send).
Breaking
get_current_lid,get_lid_for_phone, andIsOnWhatsAppUser.known_lidchangeString→CompactString.Tests
cargo test -p wacore -p whatsapp-rust(1500+ tests incl. send fan-out, lid_pn_cache, usync, history_sync) pass;clippy --all-targetsclean.