Repository navigation
perf(ios): coalesce strap-log publishes instead of one per line - #2548
Merged
Merged
Conversation
`LiveState` carries dozens of `@Published` properties on one `ObservableObject`, and an `ObservableObject` invalidates EVERY observer on ANY published change. So `@Published var log` woke every view in the app once per appended line, and a history drain plus a re-score burst emits hundreds a minute. That was the invalidation half of the background CPU kill in #2521; #2546 cut the cost of each invalidation, this cuts how many there are to at most four a second. The Android twin has throttled this for a while, with `boundedRevision(ble.logRevision, coalesceMs = 250)` in TestCentreScreen.kt. Same interval here, so the two platforms behave alike. Coalesces the PUBLISH, never the capture. `log` becomes a synchronous computed read of a non-published buffer, so code that appends and then inspects the log in the same turn still sees its own line, which many tests do. The disk archive and the #990 drained-rows tally stay per line and run before the publish. Leading edge plus a trailing flush, so the first line after an idle gap is immediate and the last line of a burst always lands rather than waiting for a line that may never come. The decision is a pure `nonisolated static func` returning an enum, so the policy is pinned without a clock or a run loop, and the async half is a thin adapter with no decisions of its own. Two isolation details, because a `static` member of a `@MainActor` type is actor-isolated: the interval constant is `nonisolated` so the nonisolated decision function can use it as a default argument, matching `legacyTailKey` in the same file. The card now observes `logRevision` rather than `log.count`. The revision is monotonic and ticks once per publish; the count plateaus while the ring trims, so it could stay equal across an append and skip a scroll.
Re-review of the previous commit. Two problems, one of them an implementation flaw. A `lastPublish` in the future makes `interval - elapsed` EXCEED the interval, so a clock that went backwards would stall the log un-notified for longer than one window (0.45s for a 0.20s backward step). The wait is now clamped into [0, interval] at both ends, not just at zero. And two tests compared doubles for equality through the enum's synthesized Equatable. The payload is the result of a floating-point subtraction: `0.25 - (100.10 - 100)` is 0.15000000000000568, so asserting `.scheduleIn(0.15)` compared two unequal values and would have failed on the macOS leg. Those now destructure and assert with an accuracy, and the enum's own doc warns about the trap so the next test written against it does the same. The boundary case now uses exactly-representable values (1.0, 1.25, 0.25) instead of 100 and 100.25. It passed before, but only because that particular subtraction rounded favourably, which is not what the test is meant to be about. Every case re-checked against the clamped arithmetic before pushing.
5 tasks done
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.
Summary
Addresses #2547.
LiveStatecarries dozens of@Publishedproperties on oneObservableObject, and anObservableObjectinvalidates every observer on any published change. So@Published var logwoke every view in the app once per appended line, and a history drain plus a re-score burst emits hundreds a minute.That is the invalidation half of the background CPU kill @pipiche38 reported in #2521. #2546 cut the cost of each invalidation by about two orders of magnitude; this cuts how many there are, to at most four a second however fast lines arrive.
Android has throttled this for a while, via
boundedRevision(ble.logRevision, coalesceMs = 250)inTestCentreScreen.kt. Same interval here, so the platforms behave alike rather than iOS being the outlier.The rule: coalesce the publish, never the capture
logbecomes a synchronous computed read over a non-publishedlogBuffer, and a new@Published logRevisionis what SwiftUI observes. Readinglogtherefore still returns the buffer as it is right now, so code that appends and then inspects the log in the same turn sees its own line. That is not hypothetical:AlarmReadbackDecodeTests,BatteryEstimateTraceEmitTestsand others assertlive.log.contains { ... }immediately after triggering an append, and coalescing the data rather than the notification would have broken them and lost real log lines.The disk archive and the #990 drained-rows tally stay strictly per line, and the archive write happens before the publish.
Leading edge plus a trailing flush: the first line after an idle gap publishes immediately, so a live log does not sit a quarter second behind, and the last line of a burst always lands rather than waiting for a line that may never arrive.
Why the decision is a separate pure function
logPublishDecision(now:lastPublish:flushQueued:interval:)is anonisolated static funcreturning an enum, so the whole policy is pinned by synchronous tests with no clock and no run loop, and the async half is a thin adapter that makes no decisions. The boundary case matters and is covered: elapsed equal to the interval publishes rather than scheduling, otherwise a steady stream at exactly the interval would queue a flush per line and double the work.Two details that would bite a later edit
The
try?on the sleep is not incidental.logFlushQueuedis what suppresses every other publish in the window, so the one thing that closure must always do is clear it. A cancelled sleep throws; swallowing it and falling through still clears the flag and publishes. Rewriting it astry awaitwith the error propagating would leave the flag set forever, and the log would keep capturing while the UI silently froze. There is a comment saying so at the site.Isolation. A
staticmember of a@MainActortype is actor-isolated, so the interval constant isnonisolatedin order to be usable as the default argument of thenonisolateddecision function. That is the same reasonlegacyTailKeyin the same file carries the keyword, and the same mistake that cost #2546 a CI round trip.Also
The card now observes
logRevisionrather thanlog.count. The revision is monotonic and ticks exactly once per publish; the count plateaus while the ring trims, so it could stay equal across an append and skip a scroll.Verification
swiftc -parseandpython3 Tools/doc_comment_lint.py: clean. Stated plainly because-parsedoes not type-check or check isolation, so on a Linux host it is close to no verification for Swift; the macOS leg is the real check and I audited the new symbols for isolation reachability by hand instead.StrandTestscases. Seven pin the pure policy including the boundary, a backward clock, and the never-published sentinel. Two pin the behaviour that matters: that all fifty lines of a burst are readable synchronously, and that those fifty produce one publish rather than fifty. Reverting to a per-line publish turns the second into fifty.$logpublisher subscribers anywhere, the only writers are insideappend(log:), and nothing ever assigned tolog, so making it get-only is safe.Strand/BLE/**is outside the parity ledger'sSWIFT_GLOBS, so no parity entry is expected for the new symbols.Not done here: moving
logoffLiveStateentirely, the other option in #2547. That would remove the invalidation rather than rate-limit it, but it touches every reader and is a larger change; this one is bounded and matches a shipped Android precedent.