perf(store): batch session/identity/sender-key flush into one transaction per category - #714
Merged
Conversation
…tion per category
Benchmark Results67 unchanged benchmark(s)
|
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@claude review |
|
Claude finished @jlucaso1's task in 3m 4s —— View job Claude Code Review
SummaryClean, well-scoped batching change. The three new Correctness
Suggestions (non-blocking)
VerdictLGTM. Correctness is solid, the shape mirrors a proven pattern in the codebase, and the win is documented with a real microbench. The suggestions above are quality polish, not blockers. |
- reuse the existing with_retry helper for the three put_*_batch methods instead of duplicating the permit/spawn_blocking/backoff shell (#1) - warn on sustained busy/locked retries in with_retry (#2) - capture the batch as Arc<Vec> so retries bump a refcount, not re-clone (#3) - test duplicate address within one batch (last-write-wins) (#4)
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds batch write methods to SignalStore, implements transactional SQLite batch upserts in SqliteStore (with retry logging), refactors SignalStoreCache::flush to emit batched writes, and adds a test verifying persistence and upsert semantics. ChangesBatch Signal Store Write APIs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ 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 |
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.
Problem
SignalStoreCache::flush()ran at the end of every send and wrote each dirty session, identity, and sender key one at a time viaput_session/put_identity/put_sender_key. On the SQLite backend each of those is its ownspawn_blocking+db_semaphoreacquire (a single-permit write lane) + a one-row insert in its own implicit transaction.A cold group send that establishes hundreds-to-thousands of fresh Signal sessions turns a single flush into that many
spawn_blockingdispatches and that many separate transactions, serialized on the one write lane. The codebase already proves the batched shape is correct and far cheaper (update_device_lists,store_prekeys_batch), but noput_*_batchexisted for sessions/identities/sender keys.Change
Add
put_sessions_batch,put_identities_batch, andput_sender_keys_batchto theSignalStoretrait, each with a default implementation that loops the singular calls (so in-memory, wasm, and other backends keep working unchanged). The SQLite backend overrides them following the existingstore_prekeys_batchtemplate: onewith_retry, oneconn.transactionlooping the upserts, one write-lane acquire.flush()now collects the dirty puts per category and emits one batched call each, collapsing a cold group send's flush from Nspawn_blocking+ N transactions down to about three (one per category). Deletes stay singular (not the hotspot).Addresses are passed as
Arc<str>(the cache's own key type) rather thanString, so building the batch is a refcount bump per entry instead of a string allocation, and the SQLite impl uses the shared key directly as&strin the query. The now-unusedflush_encode_bufwas removed; each entry serializes into its ownBytes(moved, not copied).Benchmark
Wall-clock, in-memory SQLite, best-of-3, writing 500 sessions:
put_session(per-entry)put_sessions_batch~3.5x for the per-flush transaction reduction. This is single-threaded in-memory, so it captures the
spawn_blocking+ per-statement transaction overhead but not the write-lane contention under concurrency or on-disk WAL, where the win is larger.Tests
put_signal_batches_persist_and_upsert(new): all three batches persist and read back, re-batching the same addresses upserts (on_conflict do_update), and empty batches short-circuit.cargo clippy --all-targets -- -D warningsclean.