perf: throttle SwiftData query-driven re-renders under heavy ingestion + fix discovery reveal crash#2105
Merged
Merged
Conversation
…state on every packet Under sustained ingestion (large mesh, TCP stress replay, reconnect node-DB dump) three views pegged the main thread at ~100% CPU because broad live queries re-evaluated their bodies on every SwiftData write: - MeshMapMK computed visiblePositionState in body — a filter plus density sort over every latest position — per invalidation. The positions are now fetched and derived on a ~2s cadence in a task, with immediate refreshes on pan/zoom settle, filter edits, tab entry, and foreground; a change-detection key still dedupes snapshot/overlay rebuilds. - FilteredNodeList rescanned the full node set every 350ms even while the Nodes tab was off-screen (TabView keeps tabs alive), and its onChange(of: allNodes.count) kept the live query hot. The scan loop now runs only while the Nodes tab is frontmost and fetches explicitly; the router node index refreshes with it. - Settings held a live query over all nodes for the admin picker, so every node write re-rendered Settings from any tab. It now reads a snapshot refreshed every 2s while the Settings tab is frontmost. Rendering is unchanged — each view still shows the same data — only the recompute cadence moved from per-write to a throttled tick.
revealSeededNodesFromDatabase spread its reveal across most of the dwell with awaits between batches while holding fetched NodeInfoEntity/PositionEntity references. Live ingestion keeps mutating the store during that window; touching a position whose row was pruned mid-reveal trapped in SwiftData's persisted-property accessor (SIGTRAP, reproduced under a TCP stress replay while running Analyze Current Preset). All values the reveal needs are now snapshotted into plain structs in one synchronous pass before the loop, so no managed object crosses an await. The loop also stops early if a device switch resets the container mid-scan instead of trapping on the dead session/result.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughChangesSnapshot-based data refreshes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Meshtastic/Services/DiscoveryScanEngine.swift`:
- Around line 1207-1208: Update the coordinate construction around latitude and
longitude to use node.latestPosition instead of node.positions.last, preserving
the existing 0.0 fallbacks when no latest position is available.
- Line 1209: Update the messageCount value in the discovery result to use
node.user?.sentMessages.count instead of the globally grouped messageCounts
lookup. Apply the same text-message filtering used by the .textMessageApp and
.textMessageCompressedApp handling so the persisted count matches the live
discovery tally, and remove the now-unnecessary global MessageEntity
fetch/grouping if it is only used for this value.
In `@Meshtastic/Views/Nodes/MeshMapMK.swift`:
- Around line 164-166: Update the change-detection key used by the guard in the
position refresh flow around appliedPositionStateKey so it incorporates every
visible position, not only a truncated subset; preserve stable ordering and
count while ensuring changes anywhere in state.positions produce a new key and
trigger refreshVisiblePositionSnapshots.
In `@Meshtastic/Views/Nodes/NodeList.swift`:
- Around line 373-385: Move the router.selectedTab == .nodes guard before the
initial refreshDisplayedNodes() call in NodeList’s .task, so no snapshot fetch
occurs off-screen. In Meshtastic/Views/Settings/Settings.swift lines 766-780,
guard both the onAppear refresh and the task’s initial refresh with
router.selectedTab == .settings; preserve the existing refresh cadence for the
visible tab.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aca76655-6314-4e83-8ddd-9bccbd0ba296
📒 Files selected for processing (4)
Meshtastic/Services/DiscoveryScanEngine.swiftMeshtastic/Views/Nodes/MeshMapMK.swiftMeshtastic/Views/Nodes/NodeList.swiftMeshtastic/Views/Settings/Settings.swift
📄 Docs staleness warningThis PR modifies user-facing Swift source files but does not update any page under Changed source files: What to check:
If this PR does not require a doc update (e.g., internal refactor, bug fix, test change), add the After updating |
…efore initial snapshot fetches - MeshMapMK: hash every visible position into the change-detection key; with the throttled refresh it's the only rebuild gate, and the old 64-position sample could leave pins beyond it permanently stale. - DiscoveryScanEngine: snapshot coordinates via latestPosition (O(1) cache, deterministic) instead of unsorted positions.last. - NodeList/Settings: the snapshot tasks re-fire on every tab switch, so the frontmost-tab guard now precedes the initial refresh — no more full node scan on unrelated tab changes.
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
Fixes the app pegging a full core (and eventually crashing) under sustained packet ingestion — reproduced against a synthetic 1,600-node DEFCON-shaped TCP stress replay at ~100 packets/second, profiled with
sampleon the simulator.The problem
Broad live
@Queryproperties re-evaluated whole view bodies on every SwiftData write, and TabView keeps all tabs alive, so the work ran no matter which tab was frontmost:body— a filter plus density sort over every latest position — on every position write. The sort comparator faults managed objects, multiplying the cost.onChange(of: allNodes.count)kept its live query hot inbody.Under load the three together saturated the main thread — the map, message lists, and every animation janked or froze.
The fix
The same pattern in all three places: views render from a throttled snapshot; nothing in
bodyreads a live query over hot entities.In before/after profiles under the same replay, the per-write body evaluations for these views drop to zero / a handful of samples; what remains is the ingestion pipeline itself (its own actor, off the main thread — follow-up material: per-message saves + WAL checkpoints could batch).
Crash fix (found by the same stress run)
Local Mesh Discovery → Analyze Current Preset SIGTRAPped mid-scan under load:
revealSeededNodesFromDatabasespreads its reveal across most of the dwell with awaits between batches while holding fetchedNodeInfoEntity/PositionEntityreferences. Live ingestion prunes positions during that window, and touching a destroyed entity traps in SwiftData's persisted-property accessor. All needed values are now snapshotted into plain structs in one synchronous pass before the loop, and the loop stops early if a device switch resets the container mid-scan. Verified: the same analyze that reliably crashed now completes under the same flood.Behavior notes
Summary by CodeRabbit
Performance Improvements
Bug Fixes