Skip to content

Measure where a re-score pass actually goes, before optimising the reads - #1559

Merged
ryanbr merged 1 commit into
mainfrom
perf/1538-price-read-vs-score
Aug 23, 2026
Merged

Measure where a re-score pass actually goes, before optimising the reads#1559
ryanbr merged 1 commit into
mainfrom
perf/1538-price-read-vs-score

Conversation

@ryanbr

@ryanbr ryanbr commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Pricing the "make the pass cheaper" option on #1538 got as far as static analysis can, then hit a wall. Instrumentation only — no behaviour change, no scoring change.

What the static analysis settled

Each day reads a 54-hour night window (dayStart − 30hdayStart + 24h) on a 24-hour stride. Consecutive windows overlap by 30 hours, so every row is materialised about 2.25× per pass:

Window-hours read per pass 1,134
Distinct hours in the span 534
Redundant 53%
HR rows in one window @ 1 Hz 194,400 against a limit: 200_00097% of cap

Nine windowed store reads per day survive. The day-window reads (dayHr / daySteps / dayGrav) are already sliced out of the night lists by #997/#1346, so that part of the work is done and shouldn't be re-done.

What it could not settle

Whether reads or analyzeDay dominate. The pass has only ever timed itself end to end (re-score: done … in N ms), so the ~22.6 s per night is an undifferentiated number. That single ratio decides whether narrowing the windows is worth building, and no amount of reading the code answers it.

The design this would unlock, and the one it rules out

The obvious fix does not work: reading the whole 21-day span once and slicing would hold ~1.8M HR rows in memory, and OOM on big-import libraries is a known failure of this exact path.

A sliding window would. The loop already walks newest→oldest, so it could read only the incremental 24 hours per step and drop the tail — same peak memory as today, 2.25× fewer rows materialised. That is a real design, but it is only worth building if reads actually dominate.

What this PR adds

One diagnostic line per pass, on both platforms, byte-identical:

analyzeRecent cost prep=12345ms score=6789ms
  • prep brackets the nine windowed store reads plus the session matching between them.
  • score brackets analyzeDay.

They deliberately do not sum to the pass total — pass 2, the baseline folds and the reconciliation all sit outside this loop — so the line is to be read as a ratio, which is the only thing the question needs. A day skipped for too few HR samples still counts its prep, or the tally would under-report exactly the sparse-history installs where reads dominate most.

This is the instrument-first pattern the repo already uses for questions it cannot answer from a desk — #1344 for the pass duration, #688 for the REM funnel.

How to read the result

  • prepscore → build the sliding window; the ceiling is ~53% of read time.
  • scoreprep → narrowing windows is a dead end whatever the row counts suggest, and the persistent day cache is the only remaining lever.

Verification

Android 4,229 tests, 0 failures (--no-build-cache --rerun-tasks), matching main. Doc lint clean. app-build dispatched for the app-target Swift.

Not run on hardware — which is the entire point: this exists so a real device can answer a question a desk cannot. Partial work on #1538; that issue stays open.

Pricing the "make the pass cheaper" option on #1538 got as far as static
analysis can and then hit a wall: the pass has only ever timed itself end to end,
so nobody knows whether ~22.6 s per night is spent materialising rows or inside
analyzeDay. That single ratio decides whether narrowing the read windows is worth
building at all, and no amount of reading the code answers it.

What the static analysis DID settle, and why the question matters. Each day reads
a 54-hour night window (dayStart-30h to dayStart+24h) on a 24-hour stride, so
consecutive windows overlap by 30 hours and every row is materialised about 2.25
times per pass -- 1134 window-hours read against 534 distinct. 53% of the read
volume is redundant. Nine windowed store reads per day survive; the day-window
reads (dayHr/daySteps/dayGrav) were already sliced out of the night lists by
#997/#1346, so that part is done.

The obvious fix does NOT work: reading the whole 21-day span once and slicing
would hold ~1.8M HR rows in memory, and OOM on big-import libraries is a known
failure of this path. A sliding window would -- the loop already walks
newest-to-oldest, so it could read only the incremental 24 hours per step and
drop the tail, keeping today's peak memory while materialising 2.25x fewer rows.
That is a real design, but it is only worth building if reads actually dominate.

So: time the two phases. `prep` brackets the nine windowed reads plus the session
matching between them; `score` brackets analyzeDay. They deliberately do not sum
to the pass total -- pass 2, the baseline folds and the reconciliation are all
outside this loop -- so the line is to be read as a RATIO, which is the only
thing the question needs. A day skipped for too few HR samples still counts its
prep, or the tally would under-report exactly the sparse-history installs where
reads dominate most.

Instrumentation only: no behaviour change, no scoring change, one new diagnostic
line per pass on both platforms, byte-identical between them. This is the
instrument-first pattern the repo already uses for questions it cannot answer
from a desk (#1344 for the pass duration, #688 for the REM funnel).

Verified: Android 4229 tests, 0 failures (--no-build-cache --rerun-tasks),
matching main; doc lint clean. app-build to follow for the app-target Swift.
@ryanbr

ryanbr commented Aug 23, 2026

Copy link
Copy Markdown
Owner Author

Re-reviewed the code. No changes — the timer accounting verified complete, and two limitations recorded rather than papered over.

Accounting is closed on both platforms

The risk in an instrument like this is a path that starts a timer and never accounts for it, which silently under-reports one phase and skews the very ratio the PR exists to produce. Traced every exit:

Swift Kotlin
Cache-hit continue line 833 — before tPrep0 (843) line 675 — before tPrep0 (682)
Only other continue in range 850, the ≥200-sample guard — accounted 694, the MIN_HR_SAMPLES guard — accounted
score add immediately after analyzeDay's closing paren same

So a reused day contributes nothing to either phase (it does no reads and no scoring — correct), a day skipped for too few samples contributes its read time but no score time (correct — it paid for the read), and no day can start a timer that goes unrecorded. analyzeDay is a single expression with the add on the next line, so nothing can exit between capture and accounting.

The emitted strings are byte-identical across platforms, both truncating rather than rounding.

Two limitations, deliberately not changed

Different clock sources. Kotlin uses System.nanoTime() (monotonic); Swift uses Date() (wall clock), matching how the pass total has measured itself since #1344. An NTP step mid-pass would corrupt the Swift figure. The exposure is pre-existing and consistent within the file, and switching Swift to a monotonic clock is a change I cannot compile in this environment — so it is flagged rather than made. Say the word if you would rather have it monotonic before this runs on a device.

No denominator on the cost line. prep=…ms score=…ms carries no day count of its own. The ratio is what the decision needs, and the dayCache reused=N/C … days=D line immediately above supplies counts — but two logs from different installs are not directly comparable on magnitude alone.

Neither is worth uncompiled churn while the PR is green and waiting on a hardware measurement.

Unchanged from before

Android CI built and tested this (build-and-test, 4m 8s), app-build compiled both Apple targets green, and the local suite ran 4,229/0. No CI was run for this review pass.

@ryanbr
ryanbr merged commit 3e83c99 into main Aug 23, 2026
5 checks passed
@ryanbr
ryanbr deleted the perf/1538-price-read-vs-score branch August 23, 2026 07:22
ryanbr added a commit that referenced this pull request Aug 25, 2026
Curated notes for docs/releases/v10.6.0.md covering the 83 merged PRs since
v10.5.0, then Tools/appchangelog-gen.py run over them to insert the entry into
both AppChangelog.swift and AppChangelog.kt and localize the card title into all
six Android locales.

Generated rather than hand-written on purpose: the release workflow only runs
appchangelog-gen when it performs the version bump itself, so running it here
means the in-app card can be reviewed and lint-checked before anything is
published instead of arriving as a CI warning nobody reads.

Headline items are the selectable Banister TRIMP Effort scale (#1562, #1563),
the background re-score that can now finish rather than restarting forever
(#1557, #1559), the Oura ring being handed out of daytime-HR mode so its own
sleep suite can run (#1526, #1550), ramp-aware sleep with the night's HR line
(#1551, #1552), live-workout pause/discard plus SDNN on Android (#1533, #1535),
and the WHOOP 4.0 device key no longer reaching a shared strap log (#1610).

Third-party contributors credited by handle per CLAUDE.md, covering both merged
PRs and the reports behind the fixes.

The version itself is deliberately NOT bumped here: the release workflow skips
appchangelog-gen when the source version already equals the target, so
pre-bumping would ship a release whose in-app What's New still advertised the
previous one.

Verified: compileFullDebugKotlin, lintVitalFullRelease -PstagingRelease (the
ExtraTranslation gate that compile and the i18n audit both miss), i18n_audit
--ci, doc_comment_lint, and the 108 Tools tests.
xxblyxx added a commit to xxblyxx/noop that referenced this pull request Sep 1, 2026
…he cache

Three diagnostics, no behaviour change. Every later commit in the
analyze-pass-cost plan is gated on what these measure — the two prior plans
in this area both correctly refused to optimise an unprofiled hotspot, and
this is what stops that refusal from blocking a third time.

Ports the Swift half of two upstream changes this fork never took, plus one
addition of our own:

* `analyzeRecent cost prep=Nms score=Nms` (upstream ryanbr#1559, 25+/0-, ported
  near-verbatim). The pass has only ever timed itself end to end, so whether
  the ~33 s/night measured on this device is spent in the windowed store
  reads or inside analyzeDay is unknown — and that ratio is what decides
  whether narrowing the 54h-window-on-a-24h-stride reads is worth building.
  Keeps upstream's sparse-day accounting: the `hr.count >= 200` early
  `continue` folds its elapsed prep in before continuing, because 13 of the
  21 day slots on this device take that path and would otherwise vanish
  from the tally. A cache hit contributes to neither phase — the brackets
  start after the reuse `continue`, which is the point of a hit.

* An honest reuse denominator (upstream ryanbr#1556's other half): the ratio is
  now `reused/(reused+cacheable)` with `days=` alongside. `maxDays` counted
  loop iterations, so a store holding 8 real nights in a 21-day window could
  never report better than 8/21 — which reads as a broken cache and is in
  fact a healthy one. That misreading already cost one investigation.
  Upstream's attribution half is deliberately NOT ported: it infers the
  trigger from two booleans, where we already carry a real AnalyzeTrigger.

* `analyzeRecent dayCache DROPPED — sig changed: <names>` (ours). The
  2026-08-26 device log shows a full cold pass (775 s) followed by a second
  full cold pass (266 s) whose 7 cached days were byte-identical, then a
  warm pass reusing 7 of 8. Only a whole-cache drop explains the middle one,
  but the log could not say which of the 14 signature components moved — or
  whether it was a signature drop at all rather than per-day eligibility.
  The signature is now carried as (name, value) pairs and the drop names the
  components that changed. NAMES ONLY, never values: a component can carry
  profile data (age, sex). A cold process is reported as such rather than
  listing all 14. `eligible=` and `ownerFamilyNil=` on the reuse line settle
  the eligibility half — upstream hit the same class of silent registry
  absence in ryanbr#1567 and reached for the same remedy.

The signature's joined value is byte-identical to before (same components,
same order, same separator), so this restructure invalidates no cache.

Verification: `xcodegen generate` + `xcodebuild -scheme NOOPiOS
-destination generic/platform=iOS build` — BUILD SUCCEEDED. No CI covers
these targets (app-build.yml triggers only on pull_request to main, and work
here merges locally), so the local build is the gate. Not yet observed on
device; the next morning's log is what these lines exist to produce.

Kotlin twin: not required. Diagnostic strings on the iOS log sink —
docs/CROSS_PLATFORM.md binds decoders, analytics formulas, migrations and
stored values, and a log line is none of those. Same reasoning and precedent
as the rescore-floor plan's Commit 1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017g6WqitM2Y76kkYsHzpXab
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