perf: trim per-message copies on the crypto, cache, registry and storage paths - #1388
Conversation
…age paths Measured on the release `demo` example: stripped size -14.3 KiB, .text -14.7 KiB. Every change keeps behaviour and public API byte-identical. Crypto (wacore-libsignal): - AES-CBC encrypt appended the plaintext after zero-filling the whole output span; now only the padding tail is zeroed. - AES-GCM decrypt decrypted into a scratch Vec and copied on success; it now decrypts in place on `out`'s tail and truncates back on a bad tag, so the "failure leaves `out` untouched" guarantee holds without the extra allocation and copy. - GHASH fed the carryless-multiply backend one 16-byte block per call; the full-block run is now a single `update_padded`, as the AAD and tail already were. App state (wacore-appstate): - Per-record index/content MACs used the string-dispatched `CryptographicMac` (name compare chain plus a Sha512-sized stack object); they use typed `Hmac<Sha256>`/`Hmac<Sha512>` directly. - `validate_index_mac` allocated a Vec just to compare 32 bytes; it compares a stack array. - `process_patch` sized both the SET and REMOVE MAC lists to the full mutation count although they are disjoint. Noise (wacore-noise): `NoiseState::new` is generic over the pattern; the body now lives in one non-generic inner fn instead of one copy per call site argument type. Client: - `PortableCache` removal/invalidation/expiry cloned the owned key just to delete by it; removal now takes the borrowed key. - `is_from_known_device` (every group decrypt) and the device-list load in usync/retry threw the JID away and took the two-probe user lookup; they use the namespace-aware single-probe variant that group send already uses. - The `<enc>` type attribute was parsed and validated twice per node; the parsed value is passed through, and the remaining attributes are read with one parser. - Aggregated receipt fan-out built N Receipt events even with no subscriber; it checks for a handler once before the loop, the same gate `dispatch` applies per event. SQLite storage: the session/identity/prekey write retry loops cloned the record (a session is several KiB) and address on every attempt including the first; they are now `Bytes`/`Arc<str>` shared across attempts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JfVpS69EnM2Nw8UUvEQc1c
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 10 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (13)
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 |
|
| Filename | Overview |
|---|---|
| wacore/libsignal/src/crypto/provider.rs | Reuses caller-owned output storage for CBC encryption and authenticated GCM decryption while restoring the original output length on authentication failure. |
| src/client/device_registry.rs | Adds full-JID lookup entry points that retain mapped PN/LID aliases while avoiding an unnecessary opposite-direction mapping probe. |
| src/portable_cache.rs | Generalizes map removal to borrowed keys and eliminates owned-key cloning without changing expiry revalidation. |
| storages/sqlite-storage/src/sqlite_store.rs | Shares addresses and byte records across retry closures through Arc and Bytes while preserving device-scoped Diesel writes. |
| src/message/receive.rs | Passes the already-validated EncType into payload construction to avoid parsing the type attribute twice. |
| src/receipt.rs | Skips pure aggregated receipt-event construction when the event bus has no interested handler. |
| wacore/appstate/src/hash.rs | Replaces string-dispatched MAC wrappers with equivalent typed HMAC implementations and validates index MACs without allocating. |
| wacore/appstate/src/processor.rs | Sizes disjoint SET and REMOVE MAC collections according to their actual operation counts. |
| wacore/noise/src/state.rs | Moves Noise initialization into a non-generic helper without changing protocol-name hashing or authentication. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Frames[Incoming and outgoing frames] --> Crypto[Crypto buffers and GHASH]
Frames --> Parsing[Encryption attribute parsing]
Parsing --> Registry[Device registry lookup]
Registry --> Cache[Portable cache]
Crypto --> Storage[SQLite Signal-state writes]
Parsing --> Receipts[Receipt event fan-out]
Frames --> AppState[App-state MAC and patch processing]
Reviews (1): Last reviewed commit: "perf: trim per-message copies on the cry..." | Re-trigger Greptile
There was a problem hiding this comment.
No issues found across 13 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: This is a bounded performance refactor that removes redundant allocations, copies, parsing, and event construction while preserving outputs and public APIs. The focused in-place crypto and lookup changes retain failure and lookup semantics.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Merging this PR will improve performance by 23.42%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | bench_frame_decrypt_in_place[65536] |
411.7 µs | 267.4 µs | +53.95% |
| ⚡ | Simulation | bench_frame_encrypt_in_place[65536] |
726.2 µs | 582.4 µs | +24.69% |
| ⚡ | Simulation | bench_frame_decrypt_in_place[1500] |
25.7 µs | 23.2 µs | +10.89% |
| ⚡ | Simulation | bench_frame_encrypt_in_place[1500] |
30.6 µs | 28 µs | +8.99% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/performance-memory-optimization-r30cjd (aa6f7c6) with main (3896d9c)
Footnotes
-
12 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Summary
A sweep of the per-message paths for work that is paid on every frame, decrypt or store write and buys nothing: a memset the next line overwrites, a scratch buffer copied once more on success, a key cloned only to be deleted by, an attribute parsed twice, N events built for zero subscribers. Every change keeps output and public API byte-identical; the one signature that moves is
pub(crate). Measured on the releasedemoexample under the pinned toolchain:Audit
Confirmed, and each one verified against the surrounding code rather than the pattern alone:
RustCryptoProvider::aes_256_cbc_encrypt(wacore/libsignal/src/crypto/provider.rs) didout.resize(start + encrypted_size, 0)and thencopy_from_slice(plaintext)over all but the ≤16 padding bytes. One full-message memset per Signal 1:1 encrypt (session_cipher.rs), group encrypt (group_cipher.rs) and app-state mutation encode.aes_256_gcm_decryptallocatedct.to_vec(), decrypted the scratch, verified, thenextend_from_sliceintoout: one allocation and two full-size copies per call.NoiseState::decryptalready sizes its ownVecexactly, so the pair cost two allocations for one plaintext.GcmGhash::update(aes_gcm.rs) fedghash.updateone 16-byteBlockper iteration over the full-block run. The AAD (from_keyed) and the tail (finalize) two lines away already go throughupdate_padded.UniversalHash::update_paddedon a block-multiple slice is exactlyupdate(blocks)with no padding step.generate_index_mac/generate_content_mac(wacore/appstate/src/hash.rs) built aCryptographicMac::new("HmacSha256" | "HmacSha512", key): a string compare chain and an enum whose largest variant isHmacReset<Sha512>, materialized on the stack per decoded record.keys.rsandlthash.rsin the same crate already useHmac<Sha256>directly, andhmac/sha2are already direct dependencies.validate_index_macheap-allocated a 32-byteVecper record to compare it againstexpected_mac.decode.rswent out of its way to makeRecordMacs.value_maca[u8; 32]for exactly this reason.process_patch(processor.rs) sizedadded_macs(SET only) andremoved_index_macs(REMOVE only) both topatch.mutations.len(); they are disjoint and a patch is almost always all one operation, so one allocation was pure waste.NoiseState::newispub fn new(pattern: impl AsRef<[u8]>, ..)with one production caller passing&[u8]and several test/doc callers passing&[u8; N]and[u8; 32], so the body (SHA-256 branch, state construction,authenticate) was monomorphized once per argument type.PortableCache(src/portable_cache.rs):remove,invalidate,get_or_update's expiry branch andget's expiry branch all calledfind_keyto clone an ownedK(String,Jid,Arc<str>) so thatremove_key(&K)could delete it.remove_key'sorderside is keyed by the entry's ownseq, so nothing there ever needed the owned key.is_from_known_device(src/client/device_registry.rs), which runs on every successful group decrypt and again onNoSenderKeyState, threw theJidaway and calledhas_device(&sender.user, ..), the two-proberesolve_lookup_keyspath.resolve_lookup_keys_for_jidexists precisely to skip the blind secondlid_pn_cacheprobe when the namespace is known; its doc says so andget_devices_from_registryalready relies on it. Same forload_device_record(&user_list.user.user)inusync.rs(per user of a device-list response) andhas_device(&info.requester.user, ..)inretry.rs.<enc>classification (src/message/receive.rs) readoptional_string("type"), checkedEncType::from_wire(..).is_none()and discarded the parsed value, thenEncPayload::from_parts(src/message.rs) re-scanned the attributes for"type", re-ranfrom_wire, and built three separateAttrParsers over the same node forv,stateandsession_type.src/receipt.rs): the<participants>fan-out built oneReceiptper<user>(aVec<String>, aStringclone of the id, aMessageSourcewith aJidclone) and handed each todispatch, which drops it on the floor atevent_bus.dispatchwhen nothing subscribes toEventKind::Receipt. The loop is pure event production: no state mutation, no acks, and retry/enc_rekey_retrynever use the aggregated shape, as the comment above it already states.put_session_for_device,put_identity_for_device,store_prekey,store_signed_prekey(storages/sqlite-storage/src/sqlite_store.rs): each retry loop didrecord.to_vec()once and then.clone()of theVecand the addressStringinside the loop, so the happy path (attempt 0) paid a full copy of a multi-KiB session record and the address on every Signal encrypt/decrypt.put_sessions_batchin the same file already shares anArcacross attempts.Refuted or deliberately left out, so nobody re-audits them:
base640.22/0.23,hashbrown0.15/0.17,getrandom0.2/0.4) come frombuffaandring; nothing in this workspace can collapse them. Feature sets ontokio,rustls,serde_json,chronoare already minimal for what is used.AppStateMutationMAC.value_mac: Vec<u8>re-heaps the[u8; 32]thatRecordMacsun-heaped, at bothprocess_patchcall sites. Fixing it changes a public struct and its serde shape; that is a minor-bump change, not this PR.HashState.index_value_mapis never written by production code and is cloned into everyProcessedSnapshot/PatchProcessingResult, but removing it is a storage-format migration.SessionRecorddecodes every archived session in full and drops it. The comment above it andagent_docs/signal_durability.mdexplain why the validation must stay; a non-allocating structural walk would need abuffaview API I could not confirm exists.execute_request(request.clone())insrc/download.rsclones the request per host attempt for one error log. A handful of attempts per media download, dwarfed by the transfer; not worth the diff.ProtocolAddress, thread-local crypto buffers, the hint-tape encoder, SWAR ltHash, cached HKDF-extract HMACs, exact reserves on the session serializer.Design
Crypto. CBC now does
reserve,extend_from_slice(plaintext), thenresizefor the padding tail only; the buffer handed toencrypt_paddedhas identical contents. GCM decrypt appendscttoout, decryptsout[start..]in place, and on a bad tag truncates back tostart, so the documented "failures leaveoutuntouched" contract holds by observable length rather than by a scratch buffer. GHASH's full-block run is oneupdate_paddedcall, letting the clmul/pmull backend batch.App state. The three per-record MACs use
Hmac<Sha256>/Hmac<Sha512>typed directly;generate_index_mackeeps itsVec<u8>signature and forwards to a privateindex_mac_array, whichvalidate_index_maccompares on the stack.process_patchcounts SET mutations once and sizes the two lists from that.Noise.
newkeeps its generic signature and forwards to a privatenew_inner(&[u8], &[u8]), so the body is compiled once. Handshake-only, so this is a size change, not a runtime one.Cache.
CacheInner::remove_keybecomesremove_key<Q>(&Q) where K: Borrow<Q>, the same bound every public method already carries, andfind_keyis deleted.get's expiry path loses the clone-before-drop dance; the write-side re-check on(seq, inserted_at)is unchanged.Registry.
has_device_for_jidandload_device_record_for_jidtake the single-probe lookup and share the rest of the body with the&strforms throughhas_device_in/load_device_record_in.is_from_known_device,usync.rsandretry.rsswitch to them. The bare-userhas_devicehas no production caller left and is now#[cfg(test)]; the tests that probe a user under both namespaces still use it.Enc parsing.
receive.rsbinds the parsedEncTypewithlet ... elseand passes it intofrom_owned_node/from_parts, which readv,stateandsession_typethrough oneAttrParser. The test-onlyfrom_node_refparsestypeitself so its callers are unchanged.Receipts. One
has_handler_for(EventKind::Receipt)check before the fan-out loop, the same gatedispatchapplies per event and the same idiomnode_io.rsandretry.rsalready use.Storage. The four write paths copy the record into
Bytesand the address intoArc<str>once, and each attempt refcount-clones them. Diesel binds takeas_ref()slices, asput_sessions_batchalready does.Compatibility
No public API changes.
EncPayload::from_owned_node/from_partsgain anEncTypeparameter and arepub(crate).Client::has_devicewaspub(crate)and is now test-only.NoiseState::newkeeps its generic signature.Validation
The binary-size numbers above are from two
cargo build --release --example demoruns withCARGO_PROFILE_RELEASE_STRIP=falseon the pinned nightly,strip --strip-allon a copy andsizeon the original, the same measurementscripts/ci/measure_binary_size.pytakes. Full matrix, Miri and the size gate left to CI.🤖 Generated with Claude Code
https://claude.ai/code/session_01JfVpS69EnM2Nw8UUvEQc1c
Generated by Claude Code