Skip to content

perf(signal): flush the signal cache without holding the device read-lock - #888

Merged
jlucaso1 merged 2 commits into
mainfrom
perf/flush-signal-cache-readlock
Jun 18, 2026
Merged

perf(signal): flush the signal cache without holding the device read-lock#888
jlucaso1 merged 2 commits into
mainfrom
perf/flush-signal-cache-readlock

Conversation

@jlucaso1

@jlucaso1 jlucaso1 commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

What

flush_signal_cache took the Device read() guard and held it across the entire signal_cache.flush(...) call. The guard was only used to borrow device.backend (an Arc<dyn Backend>), so this clones the backend Arc out of the device snapshot and drops the guard before flushing — the batched SQLite write now runs holding no device lock.

Why

signal_cache.flush is a batched SQLite write, and flush_signal_cache runs once per decrypted message and on most sends. Holding the device read-guard for the whole flush blocked every concurrent modify_device/process_command (a Device write) for the flush duration, creating read↔write lock contention on the per-message hot path. The backend handle is cheap to clone, so nothing needs the guard held during the I/O.

No behavior change: same backend, same flush, only the lock is no longer held across it. This mirrors the existing pattern in prekeys.rs (get_device_snapshot().backend.clone()).

Tests

cargo clippy --all --tests clean; cargo test -p whatsapp-rust passes (862).

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0cf52863-9fc6-400f-a35a-c41196e07333

📥 Commits

Reviewing files that changed from the base of the PR and between 496da5a and 30d88f6.

📒 Files selected for processing (1)
  • src/client/adapters.rs

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Improved responsiveness during bulk message processing by reducing contention during in-memory signal cache flushing, so cache maintenance no longer holds a device read-lock while writing to SQLite.

Walkthrough

Client::flush_signal_cache is updated to clone the backend Arc from a device snapshot before invoking signal_cache.flush(...), rather than holding a device read guard across the entire batched SQLite write. Error mapping and callers are unchanged.

Changes

Signal Cache Flush Lock Scope Reduction

Layer / File(s) Summary
Clone backend Arc before flush
src/client/adapters.rs
flush_signal_cache now snapshots the device, clones the backend Arc, and releases the device lock before calling signal_cache.flush(...), removing the device read guard from the duration of the batched SQLite write.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Possibly related PRs

  • oxidezap/whatsapp-rust#460: Both PRs change the signal_cache flush/integration path: this PR avoids holding device read locks when flushing signal_cache by using a backend Arc snapshot, while #460 refactors SignalStoreCache::flush and routes sender-key reads through signal_cache to reduce direct store-lock coupling.
  • oxidezap/whatsapp-rust#554: Both PRs adjust the signal-cache flush path: this PR changes Client::flush_signal_cache to avoid holding device locks during SQLite writes, while #554 changes SignalStoreCache::flush to reuse an encode buffer, directly overlapping on how cached signal data is persisted to the backend.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and accurately describes the main optimization: removing the device read-lock hold during signal cache flushing to improve performance.
Description check ✅ Passed The description clearly explains the what, why, and testing approach, directly corresponding to the changes made in the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/flush-signal-cache-readlock

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Re-trigger cubic

@github-actions

github-actions Bot commented Jun 18, 2026

Copy link
Copy Markdown

📦 Binary size report

Metric main PR Δ
bin size (stripped) 10.61 MiB 10.60 MiB -5.31 KiB (-0.05%) 🔽
bin .text 8.74 MiB 8.73 MiB -5.25 KiB (-0.06%) 🔽
bin allocated (text+data+bss) 10.61 MiB 10.60 MiB -4.00 KiB (-0.04%) 🔽
llvm-lines wacore 645,636 645,636 0
llvm-lines wacore copies 17,671 17,671 0
llvm-lines whatsapp-rust lib 657,189 657,088 -101 (-0.02%) 🔽
llvm-lines whatsapp-rust lib copies 20,013 20,013 0
deps crates (Cargo.lock) 354 354 0
.text per crate
Crate main PR Δ
.text whatsapp_rust 1.48 MiB 1.48 MiB -2.58 KiB (-0.17%) 🔽
.text wacore 544.63 KiB 544.63 KiB 0
.text wacore_binary 158.83 KiB 158.83 KiB 0
.text wacore_libsignal 170.85 KiB 170.85 KiB 0
.text wacore_appstate 35.26 KiB 35.26 KiB 0
.text wacore_noise 30.68 KiB 30.68 KiB 0
.text waproto 895.34 KiB 895.34 KiB 0
.text whatsapp_rust_sqlite_storage 206.21 KiB 206.21 KiB 0
.text whatsapp_rust_tokio_transport 33.09 KiB 33.09 KiB 0
.text whatsapp_rust_ureq_http_client 6.19 KiB 6.19 KiB 0
.text std 1.14 MiB 1.13 MiB -2.66 KiB (-0.23%) 🔽
.text other deps 4.02 MiB 4.02 MiB 0
Top movers (cargo-bloat attribution)
Crate main PR Δ
std 1.14 MiB 1.13 MiB -2.66 KiB (-0.23%)
whatsapp_rust 1.48 MiB 1.48 MiB -2.58 KiB (-0.17%)
regex_automata 1.61 KiB 4.16 KiB +2.55 KiB (+158.87%)
rustix 1.41 KiB 131 B -1.28 KiB (-90.93%)
prost 465.53 KiB 464.26 KiB -1.27 KiB (-0.27%)

Baseline: 3e704d22e (latest main run) · Head: cf7cfc2d3 · Graphs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 496da5a220

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/client/adapters.rs Outdated
@codspeed-hq

codspeed-hq Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 176 untouched benchmarks


Comparing perf/flush-signal-cache-readlock (30d88f6) with main (3e704d2)

Open in CodSpeed

@jlucaso1
jlucaso1 merged commit a8efc6c into main Jun 18, 2026
14 checks passed
@jlucaso1
jlucaso1 deleted the perf/flush-signal-cache-readlock branch June 18, 2026 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants