refactor(state): extract observable stores for better SwiftUI performance#6
refactor(state): extract observable stores for better SwiftUI performance#6
Conversation
…ance Split monolithic ProcessMonitor into three specialized @observable stores to reduce unnecessary view updates and improve filtering performance: - FilterState: centralized filter/search management with 300ms debounce - ProcessStore: process data with pre-computed portProcessMap for O(1) lookups - SystemStatsStore: system metrics with history for sparkline visualizations Key improvements: - Port lookups: O(1) via portProcessMap instead of O(n*m) array scans - Search debouncing: prevents excessive filter operations during typing - Granular observation: views only re-render when their specific store changes - History sparklines: CPU/memory metrics now show 12-point history - Pre-computed filter counts: no recalculation on every view update All stores marked @mainactor for thread safety. Includes comprehensive unit tests for each store plus performance/concurrency tests.
tanRdev
force-pushed
the
eng/performance-improvements
branch
from
588aaf3 to
fb584de
Compare
|
@greptile review |
Greptile SummaryThis PR decomposes the monolithic Key issues found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Timer as DispatchSourceTimer
participant PM as ProcessMonitor
participant PS as ProcessStore
participant SS as SystemStatsStore
participant FS as FilterState
participant DV as DetailView
participant HB as HeaderBar
Timer->>PM: fire (every 2s)
PM->>PM: refresh() — collect PIDs, CPU, ports
PM->>PS: updateProcesses(finalProcesses)
PS->>PS: sort + build portProcessMap + filterCounts
PM->>SS: updateStats(SystemStats)
SS->>SS: append to cpuHistory / memoryHistory (cap 12)
Note over HB: User types in SearchField
HB->>FS: searchText = "chro..."
FS->>FS: cancel previous debounceTask
FS->>FS: new Task.sleep(300ms)
Note over DV: searchText change → no re-render (not observed)
FS->>FS: debouncedSearchText = "chro..."
DV->>FS: filter(processStore.processes)
FS-->>DV: filtered + sorted [LucidProcess]
Last reviewed commit: fb584de |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| let finalProcesses = newProcesses.sorted() |
There was a problem hiding this comment.
Double-sort on every refresh cycle
newProcesses.sorted() is called here to produce finalProcesses, but processStore.updateProcesses(finalProcesses) sorts the already-sorted array again on its first line (let sorted = newProcesses.sorted()). Since this PR is specifically targeting performance improvements, this unnecessary O(n log n) sort on every 2-second refresh cycle is a direct regression against stated goals.
Either remove the sort here and let updateProcesses own all sorting:
| let finalProcesses = newProcesses.sorted() | |
| let finalProcesses = newProcesses |
Or remove the sort inside updateProcesses and trust that callers pre-sort. The current code does both.
| try? await Task.sleep(for: self.debounceInterval) | ||
|
|
||
| guard !Task.isCancelled else { return } | ||
|
|
||
| self.debouncedSearchText = self.searchText |
There was a problem hiding this comment.
Anti-pattern: try? swallows cancellation, then manually checks Task.isCancelled
Using try? await Task.sleep(for:) discards the CancellationError that Task.sleep throws when cancelled. Execution then falls through to the guard !Task.isCancelled check, which happens to work — but only because Task.isCancelled is still true after try? consumed the error. This is fragile: the intent of the cancellation (stop the task) is expressed twice, redundantly.
The idiomatic Swift structured concurrency approach is to let the cancellation propagate naturally by NOT silencing the error:
| try? await Task.sleep(for: self.debounceInterval) | |
| guard !Task.isCancelled else { return } | |
| self.debouncedSearchText = self.searchText | |
| try await Task.sleep(for: self.debounceInterval) | |
| self.debouncedSearchText = self.searchText |
With try await (no try?), a cancelled task exits the Task body automatically when sleep throws — no explicit guard !Task.isCancelled needed.
| func removeProcess(_ process: LucidProcess) { | ||
| processes.removeAll { $0.pid == process.pid } | ||
| processByPid.removeValue(forKey: process.pid) | ||
|
|
||
| // Update port map | ||
| for port in process.ports { | ||
| portProcessMap[port]?.removeAll { $0 == process.pid } | ||
| if portProcessMap[port]?.isEmpty == true { | ||
| portProcessMap.removeValue(forKey: port) | ||
| activePorts.removeAll { $0 == port } | ||
| } | ||
| } | ||
|
|
||
| // Recalculate counts | ||
| updateProcesses(processes) |
There was a problem hiding this comment.
Redundant manual cleanup before full rebuild
Lines 62–72 manually remove the process from processByPid, clean the portProcessMap entries, and update activePorts. However, line 75 then calls updateProcesses(processes), which rebuilds all three of these data structures from scratch — making every line of manual cleanup dead code that has zero net effect.
This creates a readability hazard: future readers may assume the intermediate state matters (e.g. for an observer or binding), when it does not. Consider simplifying to just the two lines that actually have lasting effect:
func removeProcess(_ process: LucidProcess) {
processes.removeAll { $0.pid == process.pid }
updateProcesses(processes)
}| func testConcurrentAccessToProcessStore() async { | ||
| let processes = generateProcesses(count: 100) | ||
| processStore.updateProcesses(processes) | ||
|
|
||
| await withTaskGroup(of: Void.self) { group in | ||
| // Concurrent reads | ||
| for _ in 0..<100 { | ||
| group.addTask { | ||
| _ = self.processStore.processes | ||
| _ = self.processStore.activePorts | ||
| _ = self.processStore.filterCounts | ||
| } | ||
| } | ||
|
|
||
| // Concurrent writes | ||
| for i in 0..<10 { | ||
| group.addTask { | ||
| let newProcesses = self.generateProcesses(count: 100 + i) | ||
| self.processStore.updateProcesses(newProcesses) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If we get here without crash, thread safety is working | ||
| XCTAssertTrue(true) | ||
| } |
There was a problem hiding this comment.
Thread safety test is trivially serialized — it doesn't test concurrency
ProcessStore is @MainActor, which means every group.addTask { ... } in this test is scheduled on the main actor's serial executor. The "concurrent reads" and "concurrent writes" are therefore never actually concurrent — they are interleaved in an unspecified order, but never simultaneous.
The comment // If we get here without crash, thread safety is working is misleading: this test passes purely because the main actor serializes all access, not because ProcessStore has any concurrency-safe internals. A true data race (e.g. a non-@MainActor caller accessing processes directly) would not be caught by this test. Consider either removing the misleading comment or replacing it with a note explaining that thread safety is guaranteed by @MainActor isolation, not by this test.
- Remove redundant .sorted() call in ProcessMonitor.refresh() - updateProcesses() already sorts internally, saving O(n log n) per poll cycle - Fix debounce cancellation anti-pattern in FilterState - use try await with proper CancellationError handling instead of manual isCancelled check - Remove dead cleanup code in ProcessStore.removeProcess() - manual updates were immediately overwritten by updateProcesses() call - Rename misleading test and add clarifying comment - tests @mainactor isolation safety, not true concurrency
- Make ProcessMonitor.init() nonisolated to allow initialization from non-MainActor contexts (SwiftUI @State property wrappers) - Add @mainactor to killButton(for:) and killDialogMessage(for:) in SidebarView to properly access MainActor-isolated stores
1 participant
Summary
Refactored the monolithic
ProcessMonitorinto three specialized@Observablestores to reduce unnecessary SwiftUI view updates and improve filtering performance.Architecture Changes
Before: Single
ProcessMonitorclass managing all state, causing every view to re-render on any state change.After: Composed architecture with granular observation:
Performance Improvements
portProcessMapNew Features
Files Changed
New Store Classes:
Services/FilterState.swift- Filter/search state with debouncingServices/ProcessStore.swift- Process list, port mapping, filter countsServices/SystemStatsStore.swift- System metrics with history arraysNew Test Files:
LucidTests/FilterStateTests.swiftLucidTests/ProcessStoreTests.swiftLucidTests/SystemStatsStoreTests.swiftLucidTests/PerformanceTests.swiftModified Views:
ProcessMonitor.swift- Composes three stores, proxy properties for backward compatibilityLucidApp.swift- Injects all stores into SwiftUI environmentContentView.swift- Uses FilterState for filteringHeaderBar.swift- SearchField bound to FilterState.searchTextSidebarView.swift- Uses ProcessStore for filter counts, port lookupsMetricsRowView.swift- Uses SystemStatsStore for metricsMetricCardView.swift- Added MiniSparkline componentThread Safety
All stores marked
@MainActorwith properweak selfcaptures to prevent retain cycles. History arrays capped at 12 entries to prevent unbounded growth.