perf(contacts): coalesce per-contact-frame work during a sync - #29
Merged
Conversation
The lib emits a FULL `contacts` and a FULL `discovered` snapshot on every
RESP_CONTACT frame (meshcore-ts contacts.ts:346 and :371), then re-emits
both authoritatively at END_OF_CONTACTS (:520-521). coresense reacted to
each one by doing whole-collection work, so a sync was quadratic in
sqlite statements, bytes on the wire, and renderer re-renders.
Measured at N=300 through the real adapter -> sqlite -> bus path:
before after
main-process wall clock 1,930 ms 203 ms
'discovered' broadcasts 601 2
'contacts' broadcasts 301 2
discovered_contacts writes 91,200 900
conversations_fts writes 45,450 301
JSON pushed to renderer 37.55 MB 0.16 MB
Changes:
- Add a `coalesce` helper (leading edge + one trailing run per interval,
with flush/cancel) and use it for the contacts and discovered
broadcasts, the dock-badge recompute, and the holder's persistence.
- discoveredStore.applyRadioFlags: batch the lib's on_radio/favourite
write-through into one transaction and skip rows whose flags already
match, replacing two unbatched UPDATEs per row per frame. Every other
write path invalidates the cache. Regression introduced by cab268c,
which correctly fixed stale flags but became quadratic against the
lib's per-frame emit cadence.
- holder.setContacts/setChannels no longer rewrite the whole JSON file
and DROP+rebuild conversations_fts on every call; flushed on quit.
- Memoise the discovered-name index on the array identity. It was rebuilt
once per rendered message row per websocket message, because
useIdentityHash's useMemo depends on arrays whose identity changes on
every broadcast — the main source of renderer CPU during a sync.
State stays synchronous throughout; only broadcasts and derived
persistence are coalesced, so anything reading the holder or sqlite sees
the latest value immediately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…estamp 0.7.0 coalesces the `contacts`/`discovered` snapshots during a bulk sync and adds contactUpserted/contactRemoved deltas plus a contactsSynced summary. Measured at N=300, the library now emits 1 `contacts` and 1 `discovered` for a whole sync (was 301 and 601). coresense's own coalescing stays: `contactObserved` still fires per contact by design, and ingestObservedContact projects and broadcasts the whole pool per call, so without it those 300 deltas would put the broadcast storm straight back. Measured end to end at N=300: 300 contactObserved -> 2 discovered broadcasts, 0.16 MB to the renderer. Crossing 0.6.0 also activated the channel-relay matcher rewrite, which attributes a heard repeater relay by the timestamp sealed inside the packet rather than by arrival order. sendMessage never passed it, so attribution silently fell back to a newest-first guess — the ✓xN chip landing on the wrong message when two sends were in flight. Thread `timestampUnix` from sendChannelText through to registerChannelSend. The relay test's fixture was stale in the same way: it fed the library a mesh packet with `deadbeef` as ciphertext, which the 0.6.0+ matcher now rejects outright (a failed decrypt is conclusive proof the packet is not on this channel). Build a genuinely encrypted GRP_TXT payload instead, and add a case that pins the discriminating behaviour — two sends in flight, relay the older one, assert it is not credited to the newer. That case fails without the timestamp. Note: timestampUnix has second granularity, so two sends inside the same second remain indistinguishable and fall back to newest-first. That is the library's documented behaviour, not a coresense bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A coalescer runs once per interval for as long as signals keep arriving,
so the run count is `burst_duration / interval` — it scales with how
slowly the radio feeds us, not with how many events arrive. The earlier
120ms/250ms values were chosen against a synthetic harness that
delivered all 300 contact frames in a single tick, which collapsed the
whole sync into one interval and flattered the result badly.
A real sync is ~15s (one RESP_CONTACT every ~50ms over BLE). Measured
with that pacing, 300 contacts:
120ms 1s
'discovered' broadcasts 128 17
JSON pushed to renderer 5.81 MB 0.86 MB
'contacts' stays at 1 either way — meshcore-ts 0.7.0 coalesces that
snapshot itself. 'discovered' is coresense's to bound, because it is
driven by contactObserved, which correctly still fires per contact.
Live adverts are unaffected: the leading edge still fires immediately,
so only a second change inside the same second waits.
Also pin the interval in the badge tests instead of leaning on the
production default, so changing that default can't quietly turn their
timer advances into no-ops.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing on the contact path was logged. coresense logged nothing at all, and MeshCoreSession was constructed without a `logger`, so the library fell back to its noopLogger and discarded its own iterator start/done counts. Coalescing the broadcasts then removed the last indirect signal: the emit count no longer tracks the number of contacts, so a fast sync is indistinguishable from a sync that dropped records. - Pass coresense's logger into MeshCoreSession so the protocol layer's lines (including "contacts iterator starting/done") actually surface. - Subscribe to 0.7.0's `contactsSynced` and log a summary comparing what the radio delivered against what we stored, warning when it is short. Also emitted on the bus as `contactSyncSummary` so the UI can use it. It fires after the lib flushes its snapshots, so both stores are current when it lands. - Add a per-contact ingest line at trace level, below the default, so CORESENSE_LOG_LEVEL=trace can count records without a 300-line sync burying everything else at debug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces contact-sync CPU, sqlite write amplification, and renderer re-rendering by coalescing per-contact-frame work into bounded-rate broadcasts/persistence, batching discovered flag write-through, and memoizing an expensive renderer index. It also updates meshcore-ts and fixes channel relay attribution by passing the encrypted packet timestamp through send registration, while adding logging/summary signals to make sync outcomes verifiable.
Changes:
- Introduces a
coalescehelper and applies it to contacts/discovered broadcasts, dock badge recompute, and holder persistence (contacts/channels JSON + conversations FTS rebuild). - Adds
discoveredStore.applyRadioFlagsto batch and skip redundant flag writes, plus sync-summary emission/logging and test harness support for counting sqlite writes. - Memoizes the renderer’s discovered-name index on array identity; bumps
@andyshinn/meshcore-tsto0.7.0and updates relay attribution tests/fixtures.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/renderer/lib/identity-index-cache.test.ts | Adds unit coverage for the discovered-name index memoization behavior. |
| tests/unit/notifications/badge-coalescing.test.ts | Verifies dock badge recompute is coalesced for bursty events. |
| tests/unit/main/coalesce.test.ts | Adds unit tests for the new coalescing helper contract (leading/trailing/flush/cancel). |
| tests/support/sqlite-temp.ts | Resets discovered-flag module cache when swapping temp userData DBs in tests. |
| tests/support/sql-counter.ts | Adds helper to count executed sqlite write statements for perf/regression tests. |
| tests/integration/storage/discovered-flags.test.ts | Adds integration tests for applyRadioFlags batching/skip/invalidation behavior. |
| tests/integration/outbound/channel-relay-ack.test.ts | Updates relay-ack fixture to real decryptable payload + asserts timestamp-based attribution. |
| tests/integration/inbound/contacts-iterator.test.ts | Updates contacts iterator test to flush coalesced broadcasts before asserting. |
| tests/integration/adapter/sync-summary.test.ts | Adds coverage for contact sync summary signal emitted post-sync. |
| tests/integration/adapter/sync-coalescing.test.ts | Pins “sync work is O(N), not O(N²)” via bounded emit/write assertions. |
| src/renderer/shell/rightrail/sections/peopleModel.ts | Switches to memoized discovered-name index for roster row building. |
| src/renderer/lib/identity.ts | Adds discoveredNameIndex memoized on discovered array identity. |
| src/renderer/hooks/useIdentityHash.ts | Uses discoveredNameIndex to avoid rebuilding name index per render. |
| src/main/storage/discoveredContacts.ts | Implements applyRadioFlags batching + per-pubkey flag cache invalidation. |
| src/main/state/holder.ts | Coalesces contacts/channels persistence and conversations FTS rebuild; adds flush helper. |
| src/main/state/contactSync.ts | Coalesces contacts/discovered broadcasts; adds trace logging + flush helper for tests. |
| src/main/protocol/sessionAdapter.ts | Wires meshcore-ts logger into coresense logging. |
| src/main/protocol/adapterEvents.ts | Uses applyRadioFlags + coalesced discovered emit; emits sync summary on contactsSynced. |
| src/main/notifications/index.ts | Routes badge recompute subscriptions through coalescing helper. |
| src/main/notifications/badge.ts | Adds coalesced badge recompute subscription with flush/stop. |
| src/main/messaging/sendMessage.ts | Passes timestampUnix to registerChannelSend for authoritative relay attribution. |
| src/main/index.ts | Flushes coalesced holder persistence during shutdown to avoid losing last changes. |
| src/main/events/coalesce.ts | Adds coalescing utility (leading + bounded trailing) with flush/cancel. |
| src/main/events/bus.ts | Adds contactSyncSummary event type + emitter entry. |
| pnpm-lock.yaml | Updates lockfile for @andyshinn/meshcore-ts@0.7.0 and related dependency resolution. |
| package.json | Bumps @andyshinn/meshcore-ts dependency to ^0.7.0. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… branch Two issues from review, both real. The coalesce doc claimed the helper is "the difference between O(N) and O(1) emits". It isn't, and this contradicted the call-site comments added later when the intervals were widened: run count is burst_duration / interval, and a sync's duration grows with N, so runs still grow with N — just divided by the interval rather than one per signal. Measured over a real ~15s 300-contact sync: 128 runs at 120ms, 17 at 1s. Replaced with the formula, the measurements, and the practical consequence (pick the interval against burst duration; cheaper per-run work does not reduce the count). The second sync-summary test was self-contradictory: named "flags the sync as incomplete", commented "must not report complete", and asserted complete === true. It was written expecting `delivered` to come from the END_OF_CONTACTS header, then patched to match the implementation instead of having its premise fixed — so the complete === false branch, which is the whole point of the warning, had no coverage at all. - Renamed it to what it actually pins, and asserted `delivered === 0` explicitly: delivered counts records received, not the advertised header count, so a stale header can't raise a false INCOMPLETE. - Extracted summarizeContactSync as a pure helper and unit-tested both branches. Driving complete === false through a real session would require an actual dropped-contact bug, so the seam is what makes it testable. It also documents why the check is `stored >= delivered` rather than equality — a DM placeholder can legitimately inflate `stored`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A coalesced run() threw straight out of its setTimeout, and the main process installs no uncaughtException handler, so a transient SQLite error (or a throwing bus listener) during a coalesced persist/broadcast could terminate the app. Wrap run() so a failure is logged and the cadence keeps going; the next signal recovers. rebuildConversationsIndex DELETEd conversations_fts before BEGIN, so a throw mid-rebuild left the index empty while persistNow had already cleared its dirty flag — conversation search then returned nothing for the rest of the session. Move the wipe inside the transaction so a failed rebuild rolls back to the prior index. Also count db.exec writes in the test sql-counter: reconcileOnRadio and clearDiscoveredOnly write via exec, so the "work proportional to N" guards were blind to an exec-based quadratic regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Connecting to a radio had become slow — the renderer sat at ~157% CPU while contacts loaded.
Root cause
The library emitted a full
contactsand a fulldiscoveredsnapshot on everyRESP_CONTACTframe, and coresense responded to each one by doing whole-collection work. A sync was quadratic in sqlite statements, bytes on the wire, and renderer re-renders.The coresense-side regression was
cab268c, which changed thediscoveredhandler from a pure re-read into a per-rowsetOnRadio+setFavouritewrite-through. Correct fix for a real stale-flag bug; it just became quadratic against the per-frame emit cadence.Measured, N=300 through the real adapter → sqlite → bus path
discoveredbroadcastscontactsbroadcastsdiscovered_contactswritesconversations_ftswritesRe-measured with realistic pacing (300 contacts over ~15 s, one frame per ~50 ms, which is what BLE actually does): 17 broadcasts / 0.86 MB. Broadcast count under a coalescer is
burst_duration ÷ interval, so the single-tick number above flatters it — the paced figure is the honest one.Changes
coalescehelper — leading edge, then at most one run per interval, withflush/cancel. Applied to the contacts and discovered broadcasts, the dock-badge recompute, and holder persistence. Interval 1 s.discoveredStore.applyRadioFlags— one transaction, skips rows whose flags already match. Replaces two unbatchedUPDATEs per row per frame. Every other write path invalidates the cache.holder.setContacts/setChannelsno longer rewrite the whole JSON file andDROP+rebuildconversations_ftson every call; flushed on quit.State stays synchronous throughout; only broadcasts and derived persistence are coalesced, so anything reading the holder or sqlite sees the latest value immediately.
Also here
contactObservedstill fires per contact by design, and coresense projects the whole pool per call.sendMessagenever passed it, so attribution silently fell back to a newest-first guess — the ✓×N chip landing on the wrong message with two sends in flight. The test's fixture was stale in the same way (it fed the librarydeadbeefas ciphertext, which 0.6.0+ rejects outright); it now builds a genuinely encrypted GRP_TXT payload, plus a case that fails without the timestamp.MeshCoreSessionwas built without alogger, so the library discarded its own iterator counts. Now: the library's logger is wired in,contactsSyncedproduces a summary line comparing delivered vs stored (warning when short), and a per-contact ingest line sits attrace.Verification
972 tests (10 new), typecheck and lint clean. Written test-first — each assertion was watched failing with the real quadratic numbers (81 emits, 1,760 SQL writes, 860 FTS inserts at N=40) before implementing. Before/after figures come from the same harness with
src/stashed to HEAD.Not verified on real hardware.
Known limitation
timestampUnixis second-granularity, so two channel sends inside the same second remain indistinguishable and fall back to newest-first. That is the library's documented behaviour.🤖 Generated with Claude Code