perf(signal): share the sender-key message backlog behind an Arc - #881
Conversation
A group decrypt loads the SenderKeyRecord and the load clones it (unwrap_or_clone over the cache's Arc), deep-copying the per-state skipped-key backlog (up to MAX_MESSAGE_KEYS entries). After a receiver falls behind and catches up the backlog stays populated, so every later in-order message pays the full copy even though the in-order path never reads it. Move the backlog into an Arc<Vec> so a load is a refcount bump: the in-order path leaves it shared, and a mutation (skip-ahead caching or an out-of-order removal) pays one copy-on-write via Arc::make_mut, leaving the cache's copy intact. The protobuf copy is kept empty in memory and reassembled only at serialize. Dep-free Arc-COW rather than a persistent (structural-sharing) structure: a dependency in this security-critical, size-gated, wasm/esp32-built crate isn't justified to also speed up the rare out-of-order catch-up worst case; Arc-COW captures the common recurring in-order win at zero dependency cost. New bench bench_group_in_order_decrypt_with_backlog: median ~97us -> ~33us (~3x). The out-of-order worst-case bench stays flat (the load clone becomes one make_mut clone of the same size) and the empty-backlog decrypt is unchanged.
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesSenderKeyState Arc Backlog Refactor and Benchmark
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Merging this PR will not alter performance
Performance Changes
Comparing |
What
Every group decrypt loads the
SenderKeyRecordfrom the cache, and because the cache keeps its ownArc,load_sender_keyclones the inner record (Arc::unwrap_or_clone). That deep-copies each state's skipped-key backlog (sender_message_keys, up toMAX_MESSAGE_KEYSentries). Once a receiver falls behind and catches up, the backlog stays populated, so every later in-order message pays the full copy even though the in-order path never reads the backlog at all (it only advances the chain key).This moves the backlog into an
Arc<Vec<...>>so a load is a refcount bump instead of a deep copy. The in-order path leaves theArcshared; a mutation (skip-ahead caching or an out-of-order removal) pays exactly one copy-on-write viaArc::make_mut, which leaves any sharing clone (the cache's copy) untouched. The protobufstate.sender_message_keysis kept empty in memory and reassembled only atas_protobuf(serialize), so nothing on the persistence path changes.Why Arc-COW and not a persistent (structural-sharing) structure
I deliberately did not reach for an immutable/persistent collection (e.g.
rpds). Adding a dependency to this crate is expensive here: it is security-critical crypto, it is under the binary-size gate, and it is built for wasm32 and esp32. A persistent structure would additionally make the rare out-of-order catch-up worst case cheaper, but that path is a synthetic worst case, not the recurring cost. The dep-freeArc-COW split captures the common recurring win (in-order decrypts while a backlog exists) at zero dependency cost, and the only thing it gives up is speeding up that rare catch-up, which stays exactly as it is today.Measurements
New bench
bench_group_in_order_decrypt_with_backlog: a receiver with a ~2000-key backlog decrypting one in-order message on top of it. A/B on the same machine (stashing only the impl):before (deep clone): median 97us, mean 110us, with a long allocator tail (slowest 600us)
after (Arc-COW): median 33us, mean 33us, tight (slowest 38us)
So roughly 3x faster on the median and the allocation jitter tail disappears, which is the ~82KB backlog
Vecallocation no longer happening per decrypt.No regression elsewhere: the out-of-order worst-case bench stays flat (the load-time clone simply becomes one
make_mutclone of the same size when the backlog is mutated), and the empty-backlog decrypt is unchanged (cloning an emptyVecwas already free).CodSpeed will track the new bench going forward and should show the existing group benches flat.
Safety / tests
The correctness property the whole change rests on is that mutating the loaded copy must never touch the cache's copy. Two new tests cover it:
backlog_mutation_after_clone_is_isolatedclones a state (mirroring the cache keeping its copy while the loaded record is handed out), then adds and removes keys through the loaded copy and asserts the original is unaffected.serialize_roundtrip_preserves_message_key_backlogproves the split survives persistence: it builds a backlog, asserts the in-memory protobuf copy stays empty, serializes, deserializes, and recovers every key.A
debug_assertinas_protobufguards the "protobuf copy stays empty" invariant.sender_message_keysis only accessed insidesender_keys.rs, so the invariant is fully contained.cargo test -p wacore-libsignal(117 tests) andcargo test -p whatsapp-rust --lib(827 tests) pass;cargo clippy -p wacore-libsignal --all-targets -- -D warningsis clean. The change was also verified to build and pass in isolation from other unrelated work in the tree.