Skip to content

perf: drop two redundant hot-path reads added in 10.5.1, plus two staleness/parity fixes found reviewing them - #1521

Merged
ryanbr merged 3 commits into
mainfrom
perf/rr-diag-ords-and-model-read
Aug 21, 2026
Merged

perf: drop two redundant hot-path reads added in 10.5.1, plus two staleness/parity fixes found reviewing them#1521
ryanbr merged 3 commits into
mainfrom
perf/rr-diag-ords-and-model-read

Conversation

@ryanbr

@ryanbr ryanbr commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Going back over what changed between 10.5.0 and 10.5.1 for anything worth tightening. Two things were doing avoidable work on paths that run constantly, and re-reviewing each one turned up a real bug beside it.

Everything here is a refactor of code introduced in 10.5.1 itself — nothing predates this release.

1. Build the R-R ord array once (both platforms)

The over-count branch of the sleep R-R diagnostics calls three analysers, and each was handed its own freshly mapped copy of the night's ord column. Roughly 70k elements a night, on a branch reached about 21 times per analyzeRecent, which itself runs every 15 minutes. All three expressions were byte-identical, so two of the copies were pure waste.

Hoisted above the calls. Same values in the same order, so the diagnostic strings are unchanged. (#1510)

2. Read the strap model through @AppStorage (iOS)

stepsPipelineActive read WhoopModel.persisted — a UserDefaults lookup — from inside keyMetricTile, so once per metric per body pass, on a view that recomposes with live heart rate.

@AppStorage is the idiom the rest of TodayView already uses, and SettingsView already declares this same key that way. The secondary benefit turned out to matter: the value becomes observed, so the tile re-renders when the model changes. BLEManager.persistSelectedModel writes it to UserDefaults.standard, the same suite @AppStorage binds to.

3. Android's prompt could not see a strap switch

Making the iOS read observed prompted me to check what Android does on the same event, and it could not see it at all.

stepsCalibrationPrompt reads noop.selectedWhoopModel as its first act and returns null for anything that is not a 4.0. The remember wrapping it was keyed on the three calibration values only. That key is rewritten under a composed Today screen — from Settings, and from live detection in WhoopBleClient — so switching straps left the previous answer on screen until a calibration value happened to move independently.

Added the model to the key list. It costs a loaded-map lookup per pass; what stays memoised is the rest of the prompt, which is where the cost being avoided was.

4. The unset strap model read as a 4.0 (iOS)

The parity check on (2). #1512 landed this prompt on both platforms and the two halves disagreed about one state from the first commit.

Android bails when the model key is unset (?: return null). Apple read it through WhoopModel.persisted, which falls back to .whoop4 when nothing has been selected — so a user who has never paired a strap read as a 4.0 owner. Given a scored day with no step count on it, which an import-only user has, iOS offered "Need N more days where your phone also counted steps" for a strap nobody has ever seen, and Android showed nothing.

Optional-chained now, so an unset key is not a 4.0. The key is written as soon as a strap is identified and only ever holds a WhoopModel rawValue, so every real 4.0 owner still qualifies and #1512's intent is untouched — what goes away is only the never-paired case. hasDayData stays as the second guard; it answers whether the date being shown has anything scored on it, which is a different question from whether a strap is known, and it was carrying both jobs before.

Left alone deliberately: the calibration-state terms are still OR-ed outside the family check, so leftover sample days can activate the pipeline on a non-4.0 where Android cannot. That one is a design question rather than an oversight — filed as #1523 rather than decided inside a performance PR.

One non-finding, recorded so it isn't re-raised

Android suppresses the prompt on three conditions and iOS on two: iOS omits the stepsCalibrationManual flag. That reads as a gap until you check StepsEstimateEngine.calibrate, where the manual branch is if let k = manualOverride, k > 0. manual == true therefore always carries a positive coefficient, and iOS's two-condition guard is equivalent. Android's third check is defensive, not something iOS is missing.

Verification

  • Android: compileFullDebugKotlin + testFullDebugUnitTest4189 tests, 0 failures, run --no-build-cache --rerun-tasks.
  • App-target Swift: app-build 32484140383 green on both legs at a16d885a (the branch head) — Strand (macOS) and NOOPiOS — including the Test Strand step, so StrandTests ran and passed. Default CI does not cover these targets.
  • Doc-comment lint and the i18n audit are clean.
  • The pinned parity tests (DuplicatePairRatioTest / HRVAnalyzerDuplicatePairTests) still assert the analysers' exact output strings. Note they pin the analysers, not these call sites; what protects the hoist is that all three replaced expressions were identical, so it is value-identical by construction.

Not covered by a test: the remember key in (3), since observing a Compose key needs instrumentation the repo does not have, and the gate in (4), which lives in a SwiftUI view body.

No hardware needed — nothing here touches the BLE, offload, or live-HR paths. The iOS changes read a preference that BLE writes; they do not write one.

ryanbr added 3 commits August 22, 2026 00:38
…el on hot paths

Two allocations added during the 10.5.0 -> 10.5.1 work that do not need to exist.

R-R diagnostic ords (both platforms). The over-count branch of the sleep R-R
diagnostics calls three analysers, and each was handed its own freshly mapped
copy of the night's `ord` column -- roughly 70k elements a night, on a branch
reached about 21 times per `analyzeRecent`, which itself runs every 15 minutes.
The three copies are identical by construction, so two of them were pure waste.
Built once above the calls now. Same values, same order, same diagnostic
strings -- the pinned parity tests (`DuplicatePairRatioTest` /
`HRVAnalyzerDuplicatePairTests`) still assert the exact output.

Strap-model read (iOS). `stepsPipelineActive` read `WhoopModel.persisted`, a
`UserDefaults` lookup, from inside `keyMetricTile` -- once per metric per body
pass, on a view that recomposes with live heart rate. Read through `@AppStorage`
instead, which is the idiom the rest of `TodayView` already uses. The `?? .whoop4`
fallback is preserved exactly: an absent key reads as the empty string, which is
not a valid raw value, so it lands on the same default. Secondary benefit -- the
value is now observed, so switching straps re-renders the tile instead of
waiting for an unrelated recomposition.

No behavioural change on either platform; both are refactors of work introduced
in 10.5.1 itself.

Verified: android compileFullDebugKotlin + testFullDebugUnitTest (4189 tests,
0 failures, --no-build-cache --rerun-tasks); i18n audit clean; doc lint clean.
App-target Swift (`TodayView`) validated by app-build, which default CI does not
run -- see the PR for the run.
…eads

Found re-reviewing the iOS half of this branch. `stepsCalibrationPrompt` returns
null for anything that is not a 4.0 -- it reads `noop.selectedWhoopModel` as its
first act -- but the `remember` wrapping it was keyed only on the three
calibration values. The model key is rewritten under a composed Today screen,
both from Settings and from live detection in `WhoopBleClient`, so switching
straps left the previous answer on screen until a calibration value happened to
move independently.

Adding the model to the key list costs a loaded-map lookup per pass and leaves
the rest of the prompt memoised, which is where the cost being avoided was.

This is the same defect the iOS change in this branch removes from the other
side: reading the model through `@AppStorage` makes it observed, so the tile
re-renders on a strap switch. Both platforms now react to the same event.

Not covered by a test: the repo's Android tests are JVM unit tests and this is a
Compose `remember` key, which needs instrumentation to observe.

Verified: compileFullDebugKotlin + testFullDebugUnitTest (4189 tests, 0
failures, --no-build-cache --rerun-tasks); doc lint and i18n audit clean.
…been identified (iOS)

Parity gap found reviewing this branch. #1512 landed the same prompt on both
platforms, and the two halves disagreed about one state from the first commit.

`stepsCalibrationPrompt` on Android reads the model key and bails outright when
it is unset (`?: return null`). The Apple side read it through
`WhoopModel.persisted`, which falls back to `.whoop4` when nothing has been
selected — so a user who has never paired a strap read as a 4.0 owner. Given a
scored day with no step count on it, which an import-only user has, iOS offered
"Need N more days where your phone also counted steps" for a strap nobody has
ever seen, and Android showed nothing.

Optional-chained now, so an unset key is not a 4.0. The key is written by
`BLEManager.persistSelectedModel` as soon as a strap is identified and only ever
holds a `WhoopModel` rawValue, so every real 4.0 owner still qualifies and
#1512's intent is untouched: what goes away is only the never-paired case.

`hasDayData` stays as the second guard. It answers whether the date being shown
has anything scored on it, which is a different question from whether a strap is
known, and it was carrying both jobs before.

Deliberately NOT fixed here, to keep this to the one state: the calibration-state
terms are still OR-ed outside the family check, so leftover sample days can
activate the pipeline on a non-4.0. Android cannot do that. Filing separately.

Verified: app-build (see PR). No Android change — Android already behaves this
way, which is what made the gap visible.
@ryanbr ryanbr changed the title perf: drop two redundant hot-path reads added in 10.5.1 (and fix the staleness one of them hid) perf: drop two redundant hot-path reads added in 10.5.1, plus two staleness/parity fixes found reviewing them Aug 21, 2026
@ryanbr
ryanbr merged commit 083fc75 into main Aug 21, 2026
7 checks passed
@ryanbr
ryanbr deleted the perf/rr-diag-ords-and-model-read branch August 21, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant