perf(lid-pn): skip PN->LID session migration for peers with no PN state - #677
Merged
Conversation
migrate_signal_sessions_on_lid_discovery scanned MIGRATION_DEVICE_RANGE (100) device slots per newly-learned mapping, taking two per-address locks plus a session+identity lookup each. For a freshly-resolved peer (every member of a large group on first send) all 100 slots are empty, so the whole scan is wasted. Add SignalStore::has_signal_state_for_user (sqlite EXISTS, in-memory prefix scan; default true so unimplemented backends keep the full scan) and a SignalStoreCache::has_state_for_user that checks the in-memory cache then the backend. Guard the migration loop on it: skip entirely when the PN side has no session/identity. Behavior-identical for peers with state (the loop already migrated nothing when empty), and the full migration still runs otherwise. dhat (group-send 800 fresh members, cold first send): migrate frame 10.04 GB -> 0, total run 10.07 GB -> 0.02 GB (-99.8%); 12/12 group replies delivered.
|
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 (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a ChangesSignal State Existence Query
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Collaborator
Author
|
@claude review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Claude finished @jlucaso1's task in 2m 55s —— View job Claude Code Review —
|
Benchmark Results67 unchanged benchmark(s)
|
This was referenced Jun 1, 2026
Merged
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.
What
Skip the per-device PN->LID Signal-session migration when the peer has no PN state to migrate.
migrate_signal_sessions_on_lid_discoveryruns whenever a new PN<->LID mapping is learned. It loopsMIGRATION_DEVICE_RANGE(100) device slots, and for each takes two per-address session locks plus a session and identity lookup. For a freshly-resolved peer (every member of a large group on the first send) none of those slots have any state, so the entire 100-slot scan is wasted.SignalStore::has_signal_state_for_user(user)(defaulttrue, so a backend that doesn't implement it keeps the full scan). Implemented for the in-memory backend (key prefix scan) and SQLite (EXISTSover sessions and identities, filtered by the owning device id).SignalStoreCache::has_state_for_user(user, backend)that checks the in-memory cache first (conservatively: any matching key counts) then the durable backend.Why
Profiling a cold group send to an ~800-member group (new
group-sendbench scenario) showedmigrate_signal_sessions_on_lid_discoveryas 95+ percent of all allocation: 800 members x 100 slots = 80k lock+lookup iterations, all finding nothing because the members were freshly resolved. This is invisible in latency logs but is real malloc/CPU churn (and likely part of the report's "~4s session setup").Results
dhat on the cold first group send to 800 fresh members:
migrate_signal_sessions_on_lid_discovery: 10.04 GB to 0 (-100 percent), ~5.15M allocations to ~800.Correctness / protocol
The guard only skips when there is provably nothing to migrate, which is identical to what the 100-slot loop did when every slot was empty. For peers with existing PN sessions (the migration's actual purpose, e.g. a prior DM contact who later reveals a LID) the full migration runs unchanged. This matches whatsmeow, which migrates only the sessions that exist for the user rather than a fixed device range; the guard is a conservative subset of that (skip when none).
Tests
has_signal_state_for_user_matches_by_user_prefix(in-memory): false when empty, true after a device-0 session, no false-positive for a longer user that this one prefixes, identity-only also counts.migrate_skips_when_no_pn_signal_state: a stateless peer migration creates no LID session.migration_blocks_on_per_address_session_locknow seeds a PN session so it still exercises the lock the loop takes (the guard would otherwise skip a stateless migration). Existingmigration_preserves_working_session_when_both_namespaces_presentandmigration_lock_dance_*still pass.cargo clippy --all-targets -- -D warningsclean;cargo test -p wacore -p whatsapp-rust -p whatsapp-rust-sqlite-storage(829 + 658 + 31 passing).